Make candidate template migrations faster #18025
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is a first step in improving the performance of the upgrade tool. Essentially let's cache the incoming candidate to the outgoing migration based on the given inputs. Notes: this is technically going to be slower now, because the `location` will be different for every candidate right now. In the next commits we will make sure to drop the `location` from the cache (increasing the cache hits) and checking whether the migration is safe ahead of time.
Instead of checking whether a candidate migration is safe during each migration step, we can do it all up front. To make things worse, we did check the safety in some migrations, but not in others which means that we could still be performing unsafe migrations. The big issue with "unsafe" migrations is where we use very small classes such as `blur` or `visible` and they can be used in non-Tailwind CSS contexts. Migrating these will result in unsafe migrations. The moment we use variants, modifiers, arbitrary values the chances that these are actual unsafe migrations are so low. We can use that as a heuristic to check whether a migration is safe or not. When we are dealing with `blur`, `visible` or `flex` we can apply the other, existing, safety checks. Once we know that a migration is unsafe, we will skip the migration. Bonus points: this also takes the `location` information out of the cache, which means that the cache will be smaller and increase cache hits tremendously.
Before even gathering context of the candidate's surroundings, we can already look at the candidate itself to know whether a migration will be safe. If a candidate doesn't even parse at all, we can stop early. If a candidate uses arbitrary property syntax such as `[color:red]`, there is like a 0% chance that this will cause an actual issue.. and so on.
When we do this upfront, then other migrations don't have to worry about whether a candidate is potentially _not_ in its minimal canonicalized form. Additionally, this allows us to cleanup the main migrate function itself, because we don't have to re-parse and re-print if nothing changed in the meantime.
This is now implicitly handled by the `migrateCanonicalizeCandidate` migration because we are parsing and printing the candidate.
Except if the `attr` is a `class`
When you have something like:
```
if (!duration) {}
```
It could be that we migrate this to:
```
if (duration!) {}
```
Hopefully our safety checks do not do that though. But in the event that
they do, the `!duration` should not be converted to `duration!` because
that class simply doesn't exist.
It wil parse correctly as:
```
[
{
"kind": "functional",
"root": "duration",
"modifier": null,
"value": null,
"variants": [],
"important": true,
"raw": "!duration"
}
]
```
And that's because the `duration-<number>` _does_ work and produces
output. But this on its won won't produce any output at all.
If that's the case, then we can throw away any migrations related to
this and return the original value.
When a candidate doesn't parse, we throw it away. However, if you are in a Tailwind CSS v3 project then it could be that we are dealing with classes that don't exist in Tailwind CSS v4 and will be migrated later. It could also be that we are dealing with legacy syntax, e.g.: `tw__flex` if you have a custom variant separator. This fixes that by only bailing in Tailwind CSS v4 and up.
Otherwise the normal `class` would also be considered invalid, which is not what we want.
In order to properly parse the value, we already had to make sure that `group` and `peer` exist in the framework during a Tailwind CSS v3 → Tailwind CSS v4 migration. We now also verify that the actual produced class is valid, for this we use `@apply` internally, but because of the `return null` this was considered invalid. This fixes that by making sure that we have some declarations, in this case I used a random key like `--phantom-class: group`. Then to make sure that `group` and `group/foo` are considered distinct classes, I also added the modifier to the equation.
The color is called `superRed` which is migrated to `super-red`, but we used the color `red-superRed` and expected `red-super-red` which doesn't exist in the theme and therefore wasn't migrated.
RobinMalfait
commented
May 15, 2025
Comment on lines
-122
to
-149
| for (let [fromCandidate, toCandidate, fromThemeKey, toThemeKey] of migrate(rawCandidate)) { | ||
| // Every utility that has a simple representation (e.g.: `blur`, `radius`, | ||
| // etc.`) without variants or special characters _could_ be a potential | ||
| // problem during the migration. | ||
| let isPotentialProblematicClass = (() => { | ||
| if (fromCandidate.variants.length > 0) { | ||
| return false | ||
| } | ||
|
|
||
| if (fromCandidate.kind === 'arbitrary') { | ||
| return false | ||
| } | ||
|
|
||
| if (fromCandidate.kind === 'static') { | ||
| return !fromCandidate.root.includes('-') | ||
| } | ||
|
|
||
| if (fromCandidate.kind === 'functional') { | ||
| return fromCandidate.value === null || !fromCandidate.root.includes('-') | ||
| } | ||
|
|
||
| return false | ||
| })() | ||
|
|
||
| if (location && isPotentialProblematicClass && !isSafeMigration(location)) { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
This is now handled ahead of time before attempting to even migrate
Comment on lines
-75
to
-79
| location?: { | ||
| contents: string | ||
| start: number | ||
| end: number | ||
| }, |
There was a problem hiding this comment.
The main migrate function will still receive the location, but each individual migration will not
philipp-spiess
approved these changes
May 15, 2025
There was a problem hiding this comment.
Nice! I think this makes sense. The only thought I had while going through it commit-by-commit was the fact that we know some utilities from v3 wouldn't work in v4 ootb (we do extend the design system somewhere for that IIRC) but it seems like you addressed this by opting out of that check if the migration is 3 => 4. 👍
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR makes the migrations for templates much faster. To make this work, I also had to move things around a bit (so you might want to check this PR commit by commit). I also solved an issue by restructuring the code.
Performance
For starters, we barely applied any caching when migrating candidates from α to β. The problem with this is that in big projects the same candidates will appear everywhere, so caching is going to be useful here.
One of the reasons why we didn't do any caching is that some migrations were checking if a migration is actually safe to do. To do this, we were checking the
location(the location of the candidate in the template). Since this location is unique for each template, caching was not possible.So the first order of business was to hoist the
isSafeMigrationcheck up as the very first thing we do in the migration.If we do this first, then the only remaining code relies on the
DesignSystem,UserConfigandrawCandidate.In a project, the
DesignSystemandUserConfigwill be the same during the migration, only therawCandidatewill be different which means that we can move all this logic in a good oldDefaultMapand cache the heck out of it.Running the numbers on our Tailwind Plus repo, this results in:
That's a lot of work we don't have to do. Looking at the timings, the template migration step goes from ~45s to ~10s because of this.
Another big benefit of this is that this makes migrations actually safe. Before we were checking if a migration was safe to do in specific migrations. But other migrations were still printing the candidate which could still result in an unsafe migration.
For example when migrating the
blurand theshadowclasses, theisSafeMigrationwas used. But if the input was!flexthen the safety check wasn't even checked in this specific migration.Safe migrations
Also made some changes to the
isSafeMigrationlogic itself. We used to start by checking the location, but thinking about the problem again, the actual big problem we were running into is classes that are short likeblur, andshadowbecause they could be used in other contexts than a Tailwind CSS class.Inverting this logic means that more specific Tailwind CSS classes will very likely not cause any issues at all.
For example:
hover:focus:flex[color:red]bg-[red]bg-red-500/50-in the name:bg-red-500Even better if we can't parse a candidate at all, we can skip the migrations all together.
This brings us to the issue in #17974, one of the issues was already solved by just hoisting the
isSafeMigration. But to make the issue was completely solved I also made sure that in Vue attributes like:active="…"are also considered unsafe (note::classis allowed).Last but not least, in case of the
!durationthat got replaced withduration!was solved by verifying that the candidate actually produces valid CSS. We can compute the signature for this class.The reason this wasn't thrown away earlier is because we can correctly parse
durationbutdurationon its own doesn't exist,duration-<number>does exist as a functional utility which is why it parsed in the first place.Fixes: #17974
Test plan
[ci-all] let's run this on all CI platforms...