Skip to content
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

Migrate arbitrary values to built-in values #14841

Merged
merged 9 commits into from
Nov 11, 2024

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Oct 31, 2024

This PR adds a migration where we detect arbitrary variants and try to upgrade them to built-in variants.

For example, if you are using [[data-visible]]:flex, we can convert it to data-visible:flex. We can also upgrade more advanced examples such as has-[[data-visible]]:flex to a compound variant which becomes has-data-visible:flex.

A table of example migrations:

Before After
[[data-visible]]:flex data-visible:flex
[&[data-visible]]:flex data-visible:flex
[[data-visible]&]:flex data-visible:flex
[[data-url*="example"]]:flex data-[url*="example"]:flex
[[data-url$=".com"_i]]:flex data-[url$=".com"_i]:flex
[[data-url$=.com_i]]:flex data-[url$=.com_i]:flex
[&:is([data-visible])]:flex data-visible:flex
has-[[data-visible]]:flex has-data-visible:flex
has-[&:is([data-visible])]:flex has-data-visible:flex
has-[[data-slot=description]]:flex has-data-[slot=description]:flex
has-[&:is([data-slot=description])]:flex has-data-[slot=description]:flex
has-[[aria-visible="true"]]:flex has-aria-visible:flex
has-[[aria-visible]]:flex has-aria-[visible]:flex

We can also convert combinators from [&>[data-visible]]:flex to just *:data-visible:flex and [&_[data-visible]]:flex to **:data-visible:flex.

Before After
[&>[data-visible]]:flex *:data-visible:flex
[&_>_[data-visible]]:flex *:data-visible:flex
[&_[data-visible]]:flex **:data-visible:flex

Additionally, if you have complex selectors with :not(), we can convert this to a compound not-* variant in some cases as well:

Before After
[&:nth-child(2)]:flex nth-2:flex
[&:not(:nth-child(2))]:flex not-nth-2:flex

If some of the values in nth-child(…) are a bit too complex, then we still try to convert them but to arbitrary values instead.

Before After
[&:nth-child(-n+3)]:flex nth-[-n+3]:flex
[&:not(:nth-child(-n+3))]:flex not-nth-[-n+3]:flex

This also implements some optimizations around even and odd:

Before After
[&:nth-child(odd)]:flex odd:flex
[&:not(:nth-child(odd))]:flex even:flex
[&:nth-child(even)]:flex even:flex
[&:not(:nth-child(even))]:flex odd:flex

Some examples that stay as-is:

  • has-[&>[data-visible]]:flex we can't upgrade this one because has-* is not a valid variant.
  • has-[[data-visible][data-dark]]:flex we can't upgrade this one because [data-visible][data-dark] has to be on the same element. If we convert this to has-data-visible:has-data-dark:flex then this condition will be true if an element exists with data-visible and another element exists with data-dark but we don't guarantee that they are the same element.

Running this on the Catalyst codebase results in some updates that look like this:

image image

@RobinMalfait RobinMalfait requested a review from a team as a code owner October 31, 2024 10:44
@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch from cf849fb to 1500ca7 Compare October 31, 2024 16:45
ast.nodes[0].nodes[2].type === 'attribute'
) {
ast.nodes[0].nodes = [ast.nodes[0].nodes[2]]
addChildVariant = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Only thing I can think of is that *:data-visible:{utility} is technically something like :where(& > [data-visible]) but it might be fine to ignore that detail.

If we're cool with that this all looks good.

Copy link
Member Author

@RobinMalfait RobinMalfait Nov 6, 2024

Choose a reason for hiding this comment

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

Thinking about this, I think the new value is more correct because with just [&>[data-visible]] you run into more issues: https://play.tailwindcss.com/aPcIgi2nc5

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine with making the change. Maybe should output a small note when we find it in case the old behavior is being relied on?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also be fine with doing that in a follow up PR honestly.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's an option. We technically already mention to verify the changes. What are your thoughts on this /cc @philipp-spiess @adamwathan

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's fair. Maybe that's fine.

Copy link
Member

Choose a reason for hiding this comment

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

Tricky one since the specificity is different, not sure. We probably can change it but trying to think of cases where it would matter. Should test in Catalyst which does a lot of this sort of thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are indeed cases in Catalyst where it would break if we do this conversion sadly. We could go through Catalyst and make sure that this isn't an issue, but that doesn't solve it for our users.

I'll revert this particular change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Revert this back because we made * use :is(…) instead of :where(…).

@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch 2 times, most recently from 5ce36a4 to c23cdcb Compare November 6, 2024 15:54
thecrypticace
thecrypticace previously approved these changes Nov 6, 2024
@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch 2 times, most recently from 60b5b25 to ddf193d Compare November 7, 2024 12:26
CHANGELOG.md Outdated Show resolved Hide resolved
@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch from 3d1df7f to bffd222 Compare November 8, 2024 10:00
@RobinMalfait
Copy link
Member Author

I was working on a separate PR for additional conversions similar to this. Since this is not yet merged, I think it's better to just push this one commit here as well.

@RobinMalfait
Copy link
Member Author

cb87473?diff=split&w=1

E.g.: from Catalyst

image

@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch from c092898 to b846470 Compare November 8, 2024 16:00
@RobinMalfait
Copy link
Member Author

Re-introduced the [&>[data-visible]]:flex to *:data-visible:flex change. Also added a new [&_[data-visible]]:flex to **:data-visible:flex conversion.

@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch from 224cdc7 to 65922e3 Compare November 8, 2024 16:09
@RobinMalfait RobinMalfait dismissed thecrypticace’s stale review November 8, 2024 19:04

A bunch of changes happened since this review, so I think it's best to re-review.

@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch from 65922e3 to 9919924 Compare November 8, 2024 19:04
@RobinMalfait RobinMalfait enabled auto-merge (squash) November 8, 2024 19:04
@RobinMalfait RobinMalfait force-pushed the feat/modernize-arbitrary-values branch from 9919924 to 642fa95 Compare November 9, 2024 00:56
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.

Amazing what we can do with the codemods! I did find two of the examples in the test plan slightly confusing, however I don't think they make anything worse than it is right now so LGTM

Comment on lines +46 to +48
['[[data-url*="example"]]:flex', 'data-[url*="example"]:flex'],
['[[data-url$=".com"_i]]:flex', 'data-[url$=".com"_i]:flex'],
['[[data-url$=.com_i]]:flex', 'data-[url$=.com_i]:flex'],
Copy link
Member

Choose a reason for hiding this comment

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

None of these seem to work in v3, but it is valid in v4 alpha. However I think the codemods currently are not intended to run on the alpha b/c of the variant reordering so may be able to rm support for this

https://play.tailwindcss.com/lbxFdmRt0Z

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah true, in v3 you need the &, but the main thing I was testing here is the flag placement and strings vs no strings.

E.g.: [&[data-url*="example"]]:flex


// Compound arbitrary variants
['has-[[data-visible]]:flex', 'has-data-visible:flex'],
['has-[&:is([data-visible])]:flex', 'has-data-visible:flex'],
Copy link
Member

Choose a reason for hiding this comment

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

has-[&:is([data-visible])]:flex has a specificity of 0, 3, 0 in v3 but [has-[&:is([data-visible])]:flex](has-data-visible:flex) is 0, 2, 0 in v4.

HOWEVER has-[&:is([data-visible])]:flex is also 0, 2, 0 in v4 so keeping this wouldn't fix it either.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah exactly, it's a side effect of the implementation in v4 unrelated to this change. If you want to be 100% the same, we should switch to arbitrary variants instead but that makes it worse.

@RobinMalfait RobinMalfait merged commit 0d3e13e into next Nov 11, 2024
1 check passed
@RobinMalfait RobinMalfait deleted the feat/modernize-arbitrary-values branch November 11, 2024 11:44
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.

4 participants