better splitting of selectors #1440
Conversation
🦋 Changeset detectedLatest commit: fb91382 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This is great, I suggest you create a bug report or PR on the codebase that css.ts was copied from; https://github.com/reworkcss/css/blob/master/lib/parse/index.js |
eoghanmurray
force-pushed
the
fix/css-selectors
branch
from
68c3c2e to
f653bbb
Compare
eoghanmurray
force-pushed
the
fix/css-selectors
branch
from
f653bbb to
679ffce
Compare
eoghanmurray
force-pushed
the
fix/css-selectors
branch
from
679ffce to
84f0f22
Compare
|
Unfortunately I was able to produce a further failing test by being pathological:
This PR is better than what we had before, but do you want to improve further to fix my test case? |
|
@eoghanmurray you piqued my nerd. Think the extra check to see if the selector is within an unclosed string should do the trick. Looks like there is one failing test but I'm not sure that is related |
|
Tests needed a re-run after your latest commit this morning, but I managed to add another test case that breaks things; it's possible to escape double quotes within those css strings :( |
Co-authored-by: Eoghan Murray <[email protected]>
Co-authored-by: Eoghan Murray <[email protected]>
|
I'm not permitted to merge this PR so I'll leave it up to a library maintainer |
|
@daibhin I added a question above: "Was this removed accidentally?" |
|
@eoghanmurray I couldn't find your latest question. Did you mean the final test case? I left a reply in #1440 (comment) that hopefully explains things |
This reverts commit 80c8fb1.
…trings' - I needed to escape the backslash to demonstrate the test case
- the added test case didn't actually fail before due to some voodoo with `m.replace(/,/g, '\u200C');` which took me ages to figure out. Anyhow I figure test and refactor is good in case we replace the css library
eoghanmurray
force-pushed
the
fix/css-selectors
branch
from
84b4e19 to
4091fbd
Compare
|
I've just noticed #1401 which has been merged into trunk and seems to solve a similar issue; I'll try to run the new tests from this PR against master now... |
|
|
|
I've added back the Also added another test which wasn't failing, but which 'could have been' if not for some luck elsewhere! See 4091fbd for details |
update changeset to reflect more limited effect
* better splitting of selectors - overlapping with rrweb-io#1401 * Add test from example at PostHog/posthog#21427 * ignore brackets inside selector strings * Add another test as noticed that it's possible to escape strings * Ensure we are ignoring commas within strings Co-authored-by: Eoghan Murray <[email protected]>
* better splitting of selectors - overlapping with rrweb-io#1401 * Add test from example at PostHog/posthog#21427 * ignore brackets inside selector strings * Add another test as noticed that it's possible to escape strings * Ensure we are ignoring commas within strings Co-authored-by: Eoghan Murray <[email protected]>
* better splitting of selectors - overlapping with rrweb-io#1401 * Add test from example at PostHog/posthog#21427 * ignore brackets inside selector strings * Add another test as noticed that it's possible to escape strings * Ensure we are ignoring commas within strings Co-authored-by: Eoghan Murray <[email protected]>
#169)" (#182) This reverts commit 810b39f. Reverting this in favor of rrweb-io#1401 and rrweb-io#1440 which is better than whatever it is I wrote. I kept our tests just to ensure the fix is compatible with our previous patch.
* better splitting of selectors - overlapping with rrweb-io#1401 * Add test from example at PostHog/posthog#21427 * ignore brackets inside selector strings * Add another test as noticed that it's possible to escape strings * Ensure we are ignoring commas within strings Co-authored-by: Eoghan Murray <[email protected]>
As part of the full investigation in PostHog/posthog#21427 I noticed that nested commas in selectors are not very well supported by the current
.splitregex.Moved to a JS function that only splits root selectors with a comma.
It looks to me like the
css.parseis only used to generate this AST in one place. Perhaps it's better we move this whole function over to use the already includedcssomequivalent method. Unsure if that suffers the same issues, so proposing this given I know it works but happy to explore the alternative root if it is preferred.@eoghanmurray might be best placed to help here given recent work on hover selectors