Do not migrate legacy classes with custom values #14976
Merged
Conversation
This file contains 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
|
Implemented the proper solution to compare values to ensure the migration is safe. |
RobinMalfait
force-pushed
the
feat/migrate-shadow-if-default
branch
3 times, most recently
from
f81d2b3 to
0a64140
Compare
RobinMalfait
commented
Nov 12, 2024
packages/@tailwindcss-upgrade/src/template/codemods/legacy-classes.ts
Outdated
Show resolved
Hide resolved
Comment on lines
56
to
98
| // Prepare design system with the unknown legacy classes | ||
| if (!SEEDED.has(designSystem)) { | ||
| for (let old in LEGACY_CLASS_MAP) { | ||
| designSystem.utilities.static(old, () => []) | ||
| } | ||
| SEEDED.add(designSystem) | ||
| } |
There was a problem hiding this comment.
Updated the comment, the reason we do need this is to make the migration easier by registering the shadow-sm (or all old classes) as a static utility so that the candidate.root is the actual class.
If we don't do this, then we have to deal with all kinds of candidate types (which might be fine). But that also means looking at the value and migrating those.
At the end of the day it's easier to migrate:
[
{
"kind": "static",
"root": "shadow-sm",
"variants": [],
"important": false,
"raw": "shadow-sm"
},
]Compared to this:
[
{
"kind": "functional",
"root": "shadow",
"modifier": null,
"value": {
"kind": "named",
"value": "sm",
"fraction": null
},
"variants": [],
"important": false,
"raw": "shadow-sm"
}
]There was a problem hiding this comment.
I wonder if this might cause issues downstream though since the designSystem is passed through the whole migration pipeline and now it has shadow-sm added everywhere wrongly, just for this migration.
RobinMalfait
force-pushed
the
feat/migrate-shadow-if-default
branch
3 times, most recently
from
c63f01b to
9f5a87a
Compare
RobinMalfait
force-pushed
the
feat/migrate-shadow-if-default
branch
from
622c4b6 to
4bf1b7a
Compare
This allows us to commit the "before" state, once you are debugging you can see the after state as a diff.
The simple legacy classes migration isn't as simple anymore now that we have to check the design system. Split it into 2 separate migrations instead.
This PR makes sure that migrations from suffix-less candidates (e.g.:
`rounded`, `blur`, `shadow`) are safe to be migrated.
In some code snippets that's not always the case.
Given the following code snippet:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded shadow blur" filter={blur ? 'url(…)' : undefined} />
}
```
Without this change, it would result in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur-sm?: boolean,
rounded-sm?: boolean,
shadow-sm?: boolean,
]
function Star({ point: [cx, cy, dim, blur-sm, rounded-sm, shadow-sm] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur-sm ? 'url(…)' : undefined} />
}
```
But with this change, it results in:
```tsx
type Star = [
x: number,
y: number,
dim?: boolean,
blur?: boolean,
rounded?: boolean,
shadow?: boolean,
]
function Star({ point: [cx, cy, dim, blur, rounded, shadow] }: { point: Star }) {
return <svg class="rounded-sm shadow-sm blur-sm" filter={blur ? 'url(…)' : undefined} />
}
```
Notice how the classes inside the `class` attribute _are_ converted, but
the ones in the types or as part of the JavaScript code (e.g.:
`filter={blur ? 'url(…)' : undefined}`) are not.
---------
Co-authored-by: Philipp Spiess <[email protected]>
Co-authored-by: Philipp Spiess <[email protected]>
Instead of creating an ad-hoc static utility for the candidate, we simplify the candidate to it's base form, then perform the change and then go back. E.g.: 1. Incoming string of `hover:blur!` 2. Is turned into the candidate AST for `hover:blur!` 3. Is simplified such that variants and important is stripped 4. Is printed as a string `blur` 5. Find replacement `blur-sm` 6. Parse into candidate AST 7. Re-apply the variants and important 8. Stringified `hover:blur-sm!`
RobinMalfait
force-pushed
the
feat/migrate-shadow-if-default
branch
from
4bf1b7a to
69280d2
Compare
philipp-spiess
approved these changes
Nov 14, 2024
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 fixes an issue where we migrated classes such as
roundedtorounded-sm(see: #14875)However, if you override the values in your
tailwind.config.jsfile, then the migration might not be correct.This PR makes sure to only migrate the classes if you haven't overridden the values in your
tailwind.config.jsfile.