-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Modify the CSP Evaluator to handle negated expressions #4441
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
base: main
Are you sure you want to change the base?
Conversation
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.
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... |
Thanks for the feedback. Just so I understand correctly, current Alpine.js wouldn't be able to handle an expression like |
It would. but 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. |
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`.
Thanks for your comments, everyone. I made the changes as recommended and pushed a new commit. |
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. |
@ekwoka I've taken out the extra function evaluation magic; wondering if anyone wants to take a second look. |
Did a merge with main and now the tests pass (was a spurious unrelated failure). Any further thoughts on integrating this? |
@calebporzio Hi Caleb, thought I'd ping you here. I love Alpine and have pushed hard for it to be used in a commercial project, but due to CSP requirements we have to use the CSP build, and I just wanted to add this small ergonomic enhancement so that in addition to "plain" attributes, we can also use negations. It's now been several months with no additional input on where this should go, so I was hoping to get your eyeballs on this. Cheers! |
@ekwoka I see you being quite active in the discussions around here, so was hoping to get your thoughts on the whole CSP build side of things; am I missing something about how I should go about moving this PR forward? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Simple and clean
Thanks; now what can we do to get it into a release? :D |
It's definitely frustrating to have to write custom negation methods for everything. (notIsOpen) It also seems more like a hacky workaround to have to do this for CSP builds. I'm looking forward to seeing an update with this alteration. |
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.