Enforce pseudo-selector ordering by increasing specificity #1667
Conversation
🦋 Changeset detectedLatest commit: 7e5a666 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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 |
There was a problem hiding this comment.
Just a review on this following #1666, I think this all makes sense in conjunction with that PR. No gaps I see, but I did not fully follow the AST and code; I can give a more thorough review of required after that lands, but nothing stood out worth that review from me today.
| OnceExit(root) { | ||
| root.walkRules((rule) => { | ||
| rule.selectors = rule.selectors.map((selector) => { | ||
| if (selector.includes('._')) { |
There was a problem hiding this comment.
This isn't configured or a constant, right? Given the circular dependencies issues in the past, not worth the effort, just making sure it doesn't already exist!
There was a problem hiding this comment.
This doesn't feel right. We assume Atmoic CSS classname starts with ._. The assumption might not be correct If we change the classname pattern in the future. We might forget about this if condition here. (TBH, I don't know why we would change classname pattern in the future. Maybe restart classname compression)
I think you won't need to check selector.includes('._'), If you do enforcePseudoOrder as part of atomicifyRules.
There was a problem hiding this comment.
- Move
enforcePseudoOrderinto theatomicifyRulesstage, where we no longer need to worry about accidentally processing non-Compiled selectors
There was a problem hiding this comment.
oh hmm i suspect there is no need to move enforcePseudoOrder into atomicifyRules, as all of the PostCSS plugins should only run on Compiled CSS... haven't confirmed this though
- Confirm that plugins in
packages/css/src/plugins/never process non-Compiled CSS
There was a problem hiding this comment.
Ah, well, yeah something to confirm I'm unsure unfortunately! Would depend on what is picked up in the bundler plugins.
|
|
||
| export const getPseudoSelectorScore = (selector: string): number => { | ||
| const index = styleOrder.findIndex((pseudoClass) => selector.trim().endsWith(pseudoClass)); | ||
| return index + 1; |
There was a problem hiding this comment.
I wonder if you even need the +1? I don't care much!
There was a problem hiding this comment.
yepp i plan to clean up and de-dupe these util functions once the other PR gets merged in
looks like packages/jest/ will need its own copy of these util functions though - i ran into the same problem as you did, where i can't import @compiled/utils from inside @compiled/jest
i'm hesitant to do the cleanup prematurely cuz there is overlap in the files modified across the two PRs, and dealing with merge conflicts is not fun (that was my bad for not coordinating this more efficiently)
There was a problem hiding this comment.
- De-duplicate
packages/utils/src/style-ordering.ts
dddlr
force-pushed
the
feat/pseudo-selector-sorting
branch
from
0840a34
to
5e229cd
Compare
| @@ -49,6 +51,7 @@ export const transformCss = ( | |||
| classNameCompressionMap: opts.classNameCompressionMap, | |||
There was a problem hiding this comment.
There are quite a lot of references of classNameCompression in the code base. Since we don't support this feature, we should clean them up.
I am happy to do it after this PR.
There was a problem hiding this comment.
lets not clean up classNameCompression from elsewhere in the codebase yet, as we might use it for future performance projects
dddlr
force-pushed
the
feat/pseudo-selector-sorting
branch
from
5e229cd
to
96264ed
Compare
Add the
enforcePseudoOrderoption. This ensures that the pseudo-selectors from Compiled are always applied in the order:link:visited:focus-within:focus:focus-visible:hover:activeeven when components are imported from multiple packages. This is at the expense of lengthening those selectors considerably (by adding
:not(#\\##\\##\\##\\##\\#)to the end of those selectors).This is necessary if you use multiple pseudo-selectors on the same component (e.g.
:hoverand:active), and if you are creating Compiled components that are consumed in other packages or projects.Another approach we could have taken for this was to use
@layer, but this was determined to be impractical as styles in@layerhave lower specificity than styles outside@layer, which is risky when mixing Compiled styles with non-Compiled styles. We want Compiled styles to have higher specificity! (This might change in the future depending on whether we care about using Compiled alongside other CSS solutions* or not...)This is turned off by default. To turn on, set
enforcePseudoOrdertotruein the Parcel, Webpack, or@compiled/babel-pluginconfiguration.Todos for myself
packages/css/src/plugins/__tests__/enforce-pseudo-order.ts@compiled/jesthelper function to work with this (and potentially theincreaseSpecificityoption too)*other CSS solutions = other CSS-in-JS libraries, and using plain class names (
<div className="hello" />)