-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Handle @variant inside @custom-variant
#18885
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
Conversation
828f644 to
6636897
Compare
| name, | ||
| (r) => { | ||
| let body = structuredClone(ast) | ||
| if (usesAtVariant) substituteAtVariant(body, designSystem) |
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.
This is technically evaluated lazily and every time this function is called instead of ahead of time.
But once we compute a variant we cache it anyway so should be fine.
| { | ||
| compounds: compoundsForSelectors(selectors), | ||
| }, | ||
| { compounds: compoundsForSelectors(selectors) }, |
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.
Custom variants that re-use existing @variants are not compoundable. I had an implementation where the @variant substitution was happening ahead of time to get the selectors out but we have some checks in in-* variant for example that doesn't allow nesting and @variant … is implemented using nesting...
I think this is something we can solve in a separate PR and probably after handling flattening ourselves... can of worms.
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.
This is fine imo. The problem here is the nested style rules are the things that disqualify it because otherwise we'll have to implement flattening ourselves.
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.
Exactly, could be a follow-up PR, but this PR on its own is already an improvement.
| for (let name of customVariants.keys()) { | ||
| // Pre-register the variant to ensure its position in the variant list is | ||
| // based on the order we see them in the CSS. | ||
| designSystem.variants.static(name, () => {}) |
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.
Open for suggestions here. Like maybe a designSystem.variants.reserve(name) but felt silly to introduce a new method just for this...
But my idea was to have the same behavior as-if you are overwriting internal variants that should maintain the sort order.
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.
There's an argument for keeping the position of the lastly defined @custom-variant because otherwise if you were relying on a library that now introduces a @custom-variant with the same name, the sort order will be different.
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.
i think this is fine. Keeping the last defined one would probably be more CSS-y but it would make overriding builtin variants different from custom ones and I'd prefer that they act the same.
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.
Yep agree
| let seen = new Set<Key>() | ||
| let wip = new Set<Key>() | ||
|
|
||
| let sorted: Key[] = [] |
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.
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.
Tested this with a very simple graph:
let graph = new Map<string, Set<string>>([
['A', new Set(['B', 'C'])],
['B', new Set(['D'])],
['C', new Set(['D'])],
['D', new Set(['E'])],
['E', new Set()],
])But I don't think the real graphs will be much more complex anyway...
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.
Yeah I'd keep the array in this case. The perf tradeoff makes sense when the memory hit is possibly unbounded but you're likely to hit other perf and memory problems with large CSS files first.
|
Note: we can likely use the topologicalSort utility when handling Might do a follow PR for this. |
| } | ||
|
|
||
| // Update the variant at-rule node, to be the `&` rule node | ||
| replaceWith(node) |
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.
If we never mutate the selector of the & node, then we could skip some layers by using:
| replaceWith(node) | |
| replaceWith(node.nodes) |
But the final result should be the same. This is also the same behavior we had before so I think we can keep it like this.
| // CSS-in-JS object | ||
| else if (typeof variant === 'object') { | ||
| designSystem.variants.fromAst(name, objectToAst(variant)) | ||
| designSystem.variants.fromAst(name, objectToAst(variant), designSystem) |
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.
i kinda wish we didn't have to pass the design system into something hanging off the design system :D
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.
Yeah, one way of solving this is that we put a variantFromAst onto the DesignSystem itself 🤔
or, we can pass in a callback if we want to do something with the ast (body in this case) so the logic lives in the callsite.
I think for now, this is fine because it's all private anyway.
|
Cool. |

This PR fixes an issue where you cannot use
@variantinside a@custom-variant. While you can use@variantin normal CSS, you cannot inside of@custom-variant. Today this silently fails and emits invalid CSS.Would result in:
To solve it we have 3 potential solutions:
Some important notes:
The evaluation of the
@custom-variantonly happens when you actually need it. That means that@variantinside@custom-variantwill always have the implementation of the last definition of that variant.In other words, if you use
@variant hoverinside a@custom-variant, and later you override thehovervariant, the@custom-variantwill use the new implementation.If you happen to introduce a circular dependency, then an error will be thrown during the build step.
You can consider it a bug fix or a new feature it's a bit of a gray area. But
one thing that is cool about this is that you can ship a plugin that looks like
this:
And it will use the implementation of
hoverandfocusthat the user has defined. So if they have a customhoverorfocusvariant it will just work.By default
hocus:underlinewould generate:But if you have a custom
hovervariant like:Then
hocus:underlinewould generate:Test plan
Fixes: #18524