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 multiple --value(…) or --modifier(…) calls don't delete subsequent declarations #17273

Merged
merged 5 commits into from
Mar 19, 2025

Conversation

RobinMalfait
Copy link
Member

This PR fixes a bug in the handling of @utility. Essentially if you had a declaration where you used a --modifier(…) and a --value(…) and both caused the declaration to be removed, the declaration after the current one would be removed as well.

This happened because 2 reasons:

  1. Once we removed the declaration when we handled --modifier(…), we didn't stop the walk and kept going.
  2. The replaceWith(…) code allows you to call the function multiple times but immediately mutates the AST. This means that if you call it multiple times that you are potentially removing / updating nodes followed by the current one.

E.g.:

@utility mask-r-* {
  --mask-right: linear-gradient(to left, transparent, black --value(percentage));
  --mask-right: linear-gradient(
    to left,
    transparent calc(var(--spacing) * --modifier(integer)),
    black calc(var(--spacing) * --value(integer))
  );
  mask-image: var(--mask-linear), var(--mask-radial), var(--mask-conic);
}

If this is used as mask-r-10%, then the first definition of --mask-right is kept, but the second definition of --mask-right is deleted because both --modifier(integer) and --value(integer) do not result in a valid value.

However, the mask-image declaration was also removed because the replaceWith(…) function was called twice. Once for --modifier(integer) and once for --value(integer).

Test plan

  1. Added a test to cover this case.
  2. Existing tests pass.
  3. Added a hard error if we call replaceWith(…) multiple times.

@RobinMalfait RobinMalfait requested a review from a team as a code owner March 18, 2025 18:48
Comment on lines 152 to 154
if (replacedNode) {
throw new Error('Cannot replace a node more than once')
}
Copy link
Member Author

Choose a reason for hiding this comment

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

We could also silently ignore this but then we are just hiding a bug.

Maybe we could write a better error message here that this is a Tailwind internal bug? /cc @adamwathan

Copy link
Member

Choose a reason for hiding this comment

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

Can we just not throw here? Is there something the user can do that triggers this or does it only signal a bug in Tailwind?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing the user can do and signals a bug in Tailwind for sure because we are the only ones using this API.

Only issue about silently ignoring is that we wouldn't immediately know about an issue. And worse, maybe some APIs start relying on this odd behavior.

If we had thrown here than the bug you ran into would be obvious but would also break everything 🫣

Copy link
Contributor

Choose a reason for hiding this comment

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

Just dropping in a suggestion here:

Cannot replace a node more than once. This is a bug in Tailwind CSS. Please open an issue with a reproduction at https://github.com/tailwindlabs/tailwindcss/issues

Copy link
Member

Choose a reason for hiding this comment

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

I feel like there are probably a million places in the code where we could add conditionals like this if we really wanted right? Feels weird to me that this would be the first time we ever introduced a throw to tell us we made a mistake. I don’t think we’ve ever done this any other time we fixed a bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah makes sense. I silently bailed so at least we are not mutating other AST nodes.

@RobinMalfait RobinMalfait force-pushed the fix/accidental-removal branch from 8ed9a80 to 9501584 Compare March 19, 2025 13:23
@RobinMalfait RobinMalfait enabled auto-merge (squash) March 19, 2025 13:24
@RobinMalfait RobinMalfait merged commit d698c10 into main Mar 19, 2025
6 checks passed
@RobinMalfait RobinMalfait deleted the fix/accidental-removal branch March 19, 2025 13:25
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.

3 participants