-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Template migrations: Add automatic var injection codemods #14526
Conversation
f040289
to
b378d29
Compare
In v3, some utilities didn’t automatically wrap things in var() (there’s a list in the codebase somewhere), so we should make sure not to mod those ones here 👍🏻 |
['supports-[--test]:flex', 'supports-[var(--test)]:flex'], | ||
['supports-[_--test]:flex', null], |
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 don't know if these are correct, because
supports-[--test]
maps to @supports (--test: var(--tw))
which essentially tests whether the browser understands CSS variables.
But supports-[var(--test)]
maps to @supports var(--test)
which I think is invalid? (at least I can't get it to work in Safari with any value I throw at it)
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.
Ah, I got this from your list here: https://github.com/tailwindlabs/tailwindcss/pull/13657/files#diff-5c09a7c68a4426650ba1209e3be9652052750e24131acb607e09e5efea477dacR1619-R1624
supports-[var(--test)]
mapping to @supports var(--test)
seems to be at least compatible with v3 though 🙈
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.
Hmm I see. While this is compatible with v3, it's still wrong (aka will never match). So the question is:
- Do we want to fix it (could be a breaking change since it's already broken right now)
- Do we get rid of the supports variant in this case (or the candidate entirely because it won't do anything anyway)
- Do we do what v4 expects, where it maps to
--test: var(--tw)
(which essentially is the fix from 1.) - Don't do anything
packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.test.ts
Outdated
Show resolved
Hide resolved
packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.test.ts
Show resolved
Hide resolved
packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.ts
Outdated
Show resolved
Hide resolved
…var-injection.test.ts Co-authored-by: Robin Malfait <[email protected]>
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.
Looking nice! Just a few suggestions
packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.ts
Show resolved
Hide resolved
packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.ts
Outdated
Show resolved
Hide resolved
…var-injection.ts Co-authored-by: Robin Malfait <[email protected]>
…var-injection.ts Co-authored-by: Robin Malfait <[email protected]>
In v4, we're removing automatic var injection (please refer to this PR for more detail as to why).
Automatic var injection made it so that if you have a candidate like
bg-[--my-color]
, v3 would automatically wrap the content of the arbitrary section with avar(…)
, resulting in the same as typingbg-[var(--my-color)]
.This PR adds codemods that go over various arbitrary fields and does the
var(…)
injection for you. To be precise, we will addvar(…)
to:bg-red-500/[var(--my-opacity)]
supports-[var(--test)]:flex
[color:var(--my-color)]
bg-[var(--my-color)]