-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Ensure @utility
is processed before using them
#15542
Conversation
c58b8ea
to
cf745f7
Compare
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 breaks when generating b
and c
:
@utility a {
@apply b;
}
@utility b {
@apply c;
}
@utility c {
@apply flex;
}
3f17dbb
to
1e2bb80
Compare
features |= substituteFunctions(ast, designSystem.resolveThemeValue) | ||
features |= substituteAtApply(ast, 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.
Swapped these two around, because substituteAtApply
will result in a larger tree, which means that the substituteFunctions
can operate on a smaller tree.
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.
yay
87be5ef
to
ef272ae
Compare
If you have even more `@utility` calls, then it also behaves incorrectly. This means that we likely want a topological sort instead.
A circular dependency means that we are applying in a circle, so while this changed it's just the _other_ side of the circle (are there even sides on a circle? Help?) In these cases, the errors are technically better, because this is the first node we see where the circle starts.
Now that we are substituting `@apply` in the correct order via a topological sort, it means that we don't have to try and first handle all `@utility` at-rules. This will already be handled in the correct order.
710098c
to
9b80930
Compare
This PR fixes an issue where using an
@utility
before it is defined, and if that@utility
contains@apply
, that it won't result in the expected output. But results in an@apply
rule that is not substituted. Additionally, if you have multiple levels of@apply
, we have to make sure that everything is applied (no pun intended) in the right order.Right now, the following steps are taken:
@utility
at-rules (and register them in the system as utilities).@apply
on the AST (including@utility
's ASTs) with the content of the utility.@utility
at-rules such that they are removed from the CSS output itself.The reason we do it in this order is because handling
@apply
during@utility
handling means that we could rely on another
@utility
that is defined laterand therefore the order of the utilities starts to matter. This is not a bad
thing, but the moment you import multiple CSS files or plugins, this could
become hard to manage.
Another important step is that when using
@utility foo
, the implementation creates astructuredClone
from its AST when first using the utility. The reason we do that is becausefoo
andfoo!
generate different output and we don't want to accidentally mutate the same AST. This structured clone is the start of the problem in the linked issue (#15501).If we don't do the structured clone, then substituting the
@apply
rules would work, but thenfoo
andfoo!
will generate the same output, which is bad.The linked issue has this structure:
If we follow the steps above, this would substitute
@apply bar
first, whichresults in:
But the
bar
utility, was already cloned (and cached) so now we end up with an@apply
rule that is not substituted.To properly solve this problem, we have to make sure that we collect all the
@apply
at-rules, and apply them in the correct order. To do this, we run a topological sort on them which ensures that all the dependencies are applied before substituting the current@apply
.This means that in the above example, in order to process
@apply bar
, we have to process thebar
utility first.If we run into a circular dependency, then we will throw an error like before. You'll notice that the error message in this PR is updated to a different spot. This one is a bit easier to grasp because it shows the error where the circular dependency starts not where it ends (and completes the circle). The previous message was not wrong (since it's a circle), but now it's a bit easier to reason about.
Fixes: #15501