[LiveComponent] Tokenize classes on all allowed whitespaces #1828
Conversation
carsonbot
added
Bug
|
I don't understand why |
|
Also, forgot to mention: I'd consider a regex to tokenize the string a more elegant solution. Any objections against that? |
|
Could you rebuild the packages ? (that's the origin of the CI failures) (yarn run build from the repository root i think) |
|
Of course! Didn't find anything in the docs and wasn't sure whether that's something that actually has to be done in the PR or will be done on release. |
|
Yep this would need some CONTRIB file.. i'll open an issue :) |
|
#1836 :) |
|
Do you think you could add a quick test, to prevent any future regression ? 😊 |
|
I would like to but I didn't see any browser tests in LiveComponents. Could you point me in the right direction where actual browser integration is tested for |
aleho
force-pushed
the
2.x
branch
3 times, most recently
from
292f19d to
268b2df
Compare
|
Never mind. I figured LiveComponent doesn't have any browser tests yet and tried to add them. This PR got bigger than expected, please let me know if anything's missing. |
aleho
force-pushed
the
2.x
branch
2 times, most recently
from
93353e1 to
a290b51
Compare
|
wow indeed :) I'll look in the week as soon as i can ! Thank you for the efforts :) |
|
@smnandre Any news? Did you have time to have a quick look at it? |
|
Hello @aleho ! Sorry i've been crazy busy those last weeks... Thank you very much for this! I'm not sure we need right now a full panther stack with panther :| (really not sure there, but could this test be done like the others ?) I'll let @kbond (that is his domain much more than mine to be honest) and @WebMamba check and give their opinion :)) My personal impresion: we should maybe try to make the suggested changes in a smaller PR, and address the "panther tests" in a distinct one ? 🤷 |
No worries, we all know how it is.
There are already Panther tests for Turbo, which this PR is somewhat based on. The reason I think there's no other way to "reliably" test this is that it has to be run in a JS engine in a browser to break. Being the author of this PR I might be biased though. ;)
I can do that. On the other hand, the bigger part of this PR will only affect the tests / GitHub actions, so it's not going to have a lot of impact even if it breaks (and, as I said, Panther is already used anyway). Moreover this bug fix would not have a valid test case if it is split until the other part is merged as well. |
|
This could have been a nice fix for 2.18, just saying 😉. I'm still fine with either solution! Please just tell me how you'd like to move forward, but, please, don't let this sit for another month and then another for a release. 😭 |
|
We've all been crazy busy around here these last weeks... it's an open source project you know, and we have limited time after work, family and stuff.. And believe me when i say we would all love to have more time 😄 I just poked other regulars to come review this / give you feedback ... so that will be soon i think :) |
| const previousValue = mutation.oldValue || ''; | ||
| const previousValues = previousValue.match(/(\S+)/gu) || []; |
There was a problem hiding this comment.
Could you keep juste this file
(and the generated files in dists),
and add just a test or two in assets/test/Rendering/ExternalMutationTracker.test.js ?
I'm not sure we should create and mainain an entire new "env" ... and i'm sure i don't want to need webpack locally :))
(i'm trying to slim the PR down to merge it rapidely :) )
There was a problem hiding this comment.
I added tests and made sure they break before the fixes are applied.
|
Thanks a lot Alexander! |
The class attribute can be tokenized by any white space as defined by the HTML spec.
When a class attribute contains whitespaces other than a space change tracking fails and invalid classes are applied resulting in
Uncaught (in promise) DOMException: DOMTokenList.remove: The empty string is not a valid token..