Skip to content

Do not migrate declarations in <style> blocks #18057

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

Merged
merged 3 commits into from
May 16, 2025
Merged

Conversation

RobinMalfait
Copy link
Member

This PR improves the upgrade tool by making sure that we don't migrate CSS declarations in <style>…</style> blocks.

We do this by making sure that:

  1. We detect a declaration, the current heuristic is that the candidate is:

    • Preceded by whitespace
    • Followed by a colon and whitespace
    <style>
    .foo {
        flex-shrink: 0;
       ^           ^^
    }
    </style>
  2. We are in a <style>…</style> block

    <style>
    ^^^^^^
    .foo {
        flex-shrink: 0;
    }
    </style>
    ^^^^^^^^

The reason we have these 2 checks is because just relying on the first heuristic alone, also means that we will not be migrating keys in JS objects, because they typically follow the same structure:

let classes = {
 flex: 0,
^    ^^
}

Another important thing to note is that we can't just ignore anything in between <style>…</style> blocks, because you could still be using @apply that we do want to migrate.

Last but not least, the first heuristics is not perfect either. If you are writing minified CSS then this will likely fail if there is no whitespace around the candidate.

But my current assumption is that nobody should be writing minified CSS, and minified CSS will very likely be generated and gitignored. In either situation, replacements in minified CSS will not be any worse than it is today.

I'm open to suggestions for better heuristics.

Test plan

  1. Added an integration test that verifies that we do migrate @apply and don't migrate the flex-shrink: 0; declaration.

Fixes: #17975

However, we also can't just ignore `<style></style>` blocks _if_ they
include `@apply` because we still want to migrate those instances.
@RobinMalfait RobinMalfait marked this pull request as ready for review May 16, 2025 10:36
@RobinMalfait RobinMalfait requested a review from a team as a code owner May 16, 2025 10:36
@RobinMalfait RobinMalfait requested a review from a team as a code owner May 16, 2025 10:36
// A colon followed by whitespace after the candidate
location.contents.slice(location.end, location.end + 2)?.match(/^:\s/) &&
// A `<style` block is present before the candidate
location.contents.slice(0, location.start).includes('<style') &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Note this is using <style not <style> because there could be additional attributes

Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Seems reasonable!

@RobinMalfait RobinMalfait merged commit c7d368b into main May 16, 2025
7 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-17975 branch May 16, 2025 10:53
RobinMalfait added a commit that referenced this pull request May 19, 2025
This PR improves the performance of the upgrade tool due to a regression
introduced by #18057

Essentially, we had to make sure that we are not in `<style>…</style>`
tags because we don't want to migrate declarations in there such as
`flex-shrink: 0;`

The issue with this approach is that we checked _before_ the candidate
if a `<style` cold be found and if we found an `</style>` tag after the
candidate.

We would basically do this check for every candidate that matches.

Running this on our Tailwind UI codebase, this resulted in a bit of a
slowdown:

```diff
- Before: ~13s
+  After: ~5m 39s
```

... quite the difference.

This is because we have a snapshot file that contains ~650k lines of
code. Looking for `<style>` and `</style>` tags in a file that large is
expensive, especially if we do it a lot.

I ran some numbers and that file contains ~1.8 million candidates.

Anyway, this PR fixes that by doing a few things:

1. We will compute the `<style>` and `</style>` tag positions only once
per file and cache it. This allows us to re-use this work for every
candidate that needs it.
2. We track the positions, which means that we can simply check if a
candidate's location is within any of 2 start and end tags. If so, we
skip it.

Running the numbers now gets us to:

```diff
- Before: ~5m 39s
+  After: ~9s
```

Much better!

---------

Co-authored-by: Jordan Pittman <[email protected]>
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.

@tailwindcss/upgrade - brakes valid CSS
2 participants