-
-
Notifications
You must be signed in to change notification settings - Fork 4.9k
Migrate theme(…) calls to var(…) or the modern theme(…) syntax
#14664
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
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
5084a79
fix typos
RobinMalfait c903c87
add `theme(…)` to `var(…)` migration
RobinMalfait f3243c7
update changelog
RobinMalfait e65676e
improve converting algorithm
RobinMalfait 8b00ef1
rename `Convert.None` to `Convert.All`
RobinMalfait f0ee101
re-use `ast` when using `substituteFunctionsInValue`
RobinMalfait File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
104 changes: 104 additions & 0 deletions
104
packages/@tailwindcss-upgrade/src/template/codemods/theme-to-var.test.ts
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| import { __unstable__loadDesignSystem } from '@tailwindcss/node' | ||
| import { expect, test } from 'vitest' | ||
| import { themeToVar } from './theme-to-var' | ||
|
|
||
| test.each([ | ||
| // Keep candidates that don't contain `theme(…)` or `theme(…, …)` | ||
| ['[color:red]', '[color:red]'], | ||
|
|
||
| // Convert to `var(…)` if we can resolve the path | ||
| ['[color:theme(colors.red.500)]', '[color:var(--color-red-500)]'], // Arbitrary property | ||
| ['[color:theme(colors.red.500)]/50', '[color:var(--color-red-500)]/50'], // Arbitrary property + modifier | ||
| ['bg-[theme(colors.red.500)]', 'bg-[var(--color-red-500)]'], // Arbitrary value | ||
| ['bg-[size:theme(spacing.4)]', 'bg-[size:var(--spacing-4)]'], // Arbitrary value + data type hint | ||
|
|
||
| // Convert to `var(…)` if we can resolve the path, but keep fallback values | ||
| ['bg-[theme(colors.red.500,red)]', 'bg-[var(--color-red-500,_red)]'], | ||
|
|
||
| // Keep `theme(…)` if we can't resolve the path | ||
| ['bg-[theme(colors.foo.1000)]', 'bg-[theme(colors.foo.1000)]'], | ||
|
|
||
| // Keep `theme(…)` if we can't resolve the path, but still try to convert the | ||
| // fallback value. | ||
| [ | ||
| 'bg-[theme(colors.foo.1000,theme(colors.red.500))]', | ||
| 'bg-[theme(colors.foo.1000,var(--color-red-500))]', | ||
| ], | ||
|
|
||
| // Use `theme(…)` (deeply nested) inside of a `calc(…)` function | ||
| ['text-[calc(theme(fontSize.xs)*2)]', 'text-[calc(var(--font-size-xs)_*_2)]'], | ||
|
|
||
| // Multiple `theme(… / …)` calls should result in modern syntax of `theme(…)` | ||
| // - Can't convert to `var(…)` because that would lose the modifier. | ||
| // - Can't convert to a candidate modifier because there are multiple | ||
| // `theme(…)` calls. | ||
| // | ||
| // If we really want to, we can make a fancy migration that tries to move it | ||
| // to a candidate modifier _if_ all `theme(…)` calls use the same modifier. | ||
| [ | ||
| '[color:theme(colors.red.500/50,theme(colors.blue.500/50))]', | ||
| '[color:theme(--color-red-500/50,_theme(--color-blue-500/50))]', | ||
| ], | ||
| [ | ||
| '[color:theme(colors.red.500/50,theme(colors.blue.500/50))]/50', | ||
| '[color:theme(--color-red-500/50,_theme(--color-blue-500/50))]/50', | ||
| ], | ||
|
|
||
| // Convert the `theme(…)`, but try to move the inline modifier (e.g. `50%`), | ||
| // to a candidate modifier. | ||
| // Arbitrary property, with simple percentage modifier | ||
| ['[color:theme(colors.red.500/75%)]', '[color:var(--color-red-500)]/75'], | ||
|
|
||
| // Arbitrary property, with numbers (0-1) without a unit | ||
| ['[color:theme(colors.red.500/.12)]', '[color:var(--color-red-500)]/12'], | ||
| ['[color:theme(colors.red.500/0.12)]', '[color:var(--color-red-500)]/12'], | ||
|
|
||
| // Arbitrary property, with more complex modifier (we only allow whole numbers | ||
| // as bare modifiers). Convert the complex numbers to arbitrary values instead. | ||
| ['[color:theme(colors.red.500/12.34%)]', '[color:var(--color-red-500)]/[12.34%]'], | ||
| ['[color:theme(colors.red.500/var(--opacity))]', '[color:var(--color-red-500)]/[var(--opacity)]'], | ||
| ['[color:theme(colors.red.500/.12345)]', '[color:var(--color-red-500)]/[12.345]'], | ||
| ['[color:theme(colors.red.500/50.25%)]', '[color:var(--color-red-500)]/[50.25%]'], | ||
|
|
||
| // Arbitrary value | ||
| ['bg-[theme(colors.red.500/75%)]', 'bg-[var(--color-red-500)]/75'], | ||
| ['bg-[theme(colors.red.500/12.34%)]', 'bg-[var(--color-red-500)]/[12.34%]'], | ||
|
|
||
| // Arbitrary property that already contains a modifier | ||
| ['[color:theme(colors.red.500/50%)]/50', '[color:theme(--color-red-500/50%)]/50'], | ||
|
|
||
| // Arbitrary value, where the candidate already contains a modifier | ||
| // This should still migrate the `theme(…)` syntax to the modern syntax. | ||
| ['bg-[theme(colors.red.500/50%)]/50', 'bg-[theme(--color-red-500/50%)]/50'], | ||
|
|
||
| // Variants, we can't use `var(…)` especially inside of `@media(…)`. We can | ||
| // still upgrade the `theme(…)` to the modern syntax. | ||
| ['max-[theme(spacing.4)]:flex', 'max-[theme(--spacing-4)]:flex'], | ||
|
|
||
| // This test in itself doesn't make much sense. But we need to make sure | ||
| // that this doesn't end up as the modifier in the candidate itself. | ||
| ['max-[theme(spacing.4/50)]:flex', 'max-[theme(--spacing-4/50)]:flex'], | ||
|
|
||
| // `theme(…)` calls valid in v3, but not in v4 should still be converted. | ||
| ['[--foo:theme(fontWeight.semibold)]', '[--foo:theme(fontWeight.semibold)]'], | ||
|
|
||
| // Invalid cases | ||
| ['[--foo:theme(colors.red.500/50/50)]', '[--foo:theme(colors.red.500/50/50)]'], | ||
| ['[--foo:theme(colors.red.500/50/50)]/50', '[--foo:theme(colors.red.500/50/50)]/50'], | ||
|
|
||
| // Partially invalid cases | ||
| [ | ||
| '[--foo:theme(colors.red.500/50/50)_theme(colors.blue.200)]', | ||
| '[--foo:theme(colors.red.500/50/50)_var(--color-blue-200)]', | ||
| ], | ||
| [ | ||
| '[--foo:theme(colors.red.500/50/50)_theme(colors.blue.200)]/50', | ||
| '[--foo:theme(colors.red.500/50/50)_var(--color-blue-200)]/50', | ||
| ], | ||
| ])('%s => %s', async (candidate, result) => { | ||
| let designSystem = await __unstable__loadDesignSystem('@import "tailwindcss";', { | ||
| base: __dirname, | ||
| }) | ||
|
|
||
| expect(themeToVar(designSystem, {}, candidate)).toEqual(result) | ||
| }) | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there's a class of
theme()keypaths that are valid in v3 (and are valid in our interop layer) but don't have a keypath equivalent in v4. Things like:theme(fontWeight.semibold)ortheme(cursor.progress). Let's add some tests here to see they are still migrated.Maybe what we can do is to use the dict in
default-theme.tsand resolve these to the actual values?Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While on call, one thing we noticed is that
theme(fontWeight.semibold)is hardcoded in the font-weight utility definition to be600and doesn't have a corresponding CSS variable. This means that we can't migratetheme(fontWeight.semibold)to avar(…)or even atheme(--font-weight-semibold)because there is no CSS variable. We could inline it to600directly since it's hardcoded.However, there is a catch... if you define
--font-weight-semibold: 100as a CSS variable inside your@theme { … }, then the value is not hardcoded anymore, but the CSS variable takes precedence.This means that we will migrate
theme(fontWeight.semibold)totheme(--font-weight-semibold)or evenvar(--font-weight-semibold)but only if you have this CSS variable configured.Long story short:
var(…)ortheme(--…), and remove the CSS variable from your@theme { … }later, then the defaultfontWeight.semiboldwon't apply.theme(fontWeight.semibold)becomes600. Then adding the CSS variable to your@theme { … }later, then the variable won't take effect because we already hardcoded the value to600.theme(…)calls with dot notation, some with--…notation and somevar(…)notation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the safer thing to do here is to: