Skip to content
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

Merged
merged 11 commits into from
Jan 7, 2025
Merged

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Jan 5, 2025

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:

  1. Collect all the @utility at-rules (and register them in the system as utilities).
  2. Substitute @apply on the AST (including @utility's ASTs) with the content of the utility.
  3. Delete the @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 later
and 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 a structuredClone from its AST when first using the utility. The reason we do that is because foo and foo! 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 then foo and foo! will generate the same output, which is bad.

The linked issue has this structure:

.foo {
  @apply bar;
}

@utility bar {
  @apply flex;
}

If we follow the steps above, this would substitute @apply bar first, which
results in:

.foo {
  @apply flex;
}

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 the bar 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

@RobinMalfait RobinMalfait marked this pull request as ready for review January 6, 2025 09:33
@RobinMalfait RobinMalfait requested a review from a team as a code owner January 6, 2025 09:33
Copy link
Contributor

@thecrypticace thecrypticace left a 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;
}

CHANGELOG.md Outdated Show resolved Hide resolved
Comment on lines 541 to +542
features |= substituteFunctions(ast, designSystem.resolveThemeValue)
features |= substituteAtApply(ast, designSystem)
Copy link
Member Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay

@RobinMalfait RobinMalfait force-pushed the fix/issue-15501 branch 2 times, most recently from 87be5ef to ef272ae Compare January 7, 2025 11:27
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.
@RobinMalfait RobinMalfait enabled auto-merge (squash) January 7, 2025 15:18
@RobinMalfait RobinMalfait merged commit 02cfc45 into next Jan 7, 2025
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-15501 branch January 7, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v4] utility class is not generated when also applied to body
2 participants