Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modify the CSP Evaluator to handle negated expressions #4441

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cadolphs
Copy link

This commit adds support to use straightforward negated expressions in Alpine directives in the CSP build.

While we (probably) do not want to rewrite a full CSP-save expression evaluator to handle expressions of arbitrary complexity, directives with negation are such a common use case that it makes sense to support them.

Consider, for example,

  • x-show="!loading"
  • :disabled="!valid"

In the CSP, we would have to write custom getters for each boolean attribute whose negation we might want to use, adding needless verbosity.

On the other hand, being a unary operator, hand-rolling negation is easy enough.

To test that it all works, we add relevant cypress tests to the csp spec to verify that we can negate both a plain attribute as well as the result of a function call.

This commit adds support to use straightforward negated expressions in
Alpine directives in the CSP build.

While we (probably) do not want to rewrite a full CSP-save expression
evaluator to handle expressions of arbitrary complexity, directives with
negation are such a common use case that it makes sense to support them.

Consider, for example,

* `x-show="!loading"`
* `:disabled="!valid"`

In the CSP, we would have to write custom getters for each boolean
attribute whose negation we might want to use, adding needless
verbosity.

On the other hand, being a unary operator, hand-rolling negation is easy
enough.

To test that it all works, we add relevant cypress tests to the csp
spec to verify that we can negate both a plain attribute as well as the
result of a function call.
@ekwoka
Copy link
Contributor

ekwoka commented Nov 24, 2024

The part for "negation of a function call" I think is problematic. As this would now introduce unique behavior to the CSP build that doesn't work in Alpine standard, or in javascript.

🤔

It introduces a bit of unnecessary magic I think...

@cadolphs
Copy link
Author

Thanks for the feedback. Just so I understand correctly, current Alpine.js wouldn't be able to handle an expression like !foo() where foo is a function returning a bool?

@ekwoka
Copy link
Contributor

ekwoka commented Nov 25, 2024

Thanks for the feedback. Just so I understand correctly, current Alpine.js wouldn't be able to handle an expression like !foo() where foo is a function returning a bool?

It would.

but !foo with foo being a function would be false. It would not evaluate foo.

Here, your case seems to be running the function, and then negativing it's result. Based on that second test.

That is different results than the normal evaluator.

@SimoTod
Copy link
Collaborator

SimoTod commented Nov 25, 2024

I think you can apply the negation to evaluatedExpression before passing it into runIfTypeOfFunction to preserve the same behaviour as the non-CSP built

@cadolphs
Copy link
Author

I think you can apply the negation to evaluatedExpression before passing it into runIfTypeOfFunction to preserve the same behaviour as the non-CSP built

That's what I did initially, and then I started overthinking it, assuming that I needed to support the function-calling thing.

To keep feature parity with the Alpine standard build, we remove
function evaluation of negated function calls (`x-show=!foo`) where foo
is a function.

Also adjust the test so it checks for proper trimming and modify the
trimming of the expression to be evaluated to allow for whitespace
between the negation operatior and the expression: `x-show="! foo`.
@cadolphs
Copy link
Author

Thanks for your comments, everyone. I made the changes as recommended and pushed a new commit.

@ekwoka
Copy link
Contributor

ekwoka commented Nov 26, 2024

Yeah, I could see the motivation for it, and it could make sense in a strict "built from nothing" situation.

But in this case, divergent behaviors would be an issue.

@cadolphs cadolphs requested a review from levu42 December 1, 2024 03:27
@cadolphs
Copy link
Author

@ekwoka I've taken out the extra function evaluation magic; wondering if anyone wants to take a second look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants