Skip to content

Migrate arbitrary values to built-in values #14841

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.

Sign up for GitHub

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
RobinMalfait merged 9 commits into from
Nov 11, 2024
Merged

Migrate arbitrary values to built-in values #14841

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
44cabf1
migrate arbitrary values to modern values
RobinMalfait Oct 30, 2024
50f0a01
update changelog
RobinMalfait Oct 31, 2024
4805466
do not convert `[&>…]` to `*:…`
RobinMalfait Nov 8, 2024
4875832
migrate `:first-child`, `:last-child`, `:nth-child(…)` and so on
RobinMalfait Nov 7, 2024
a712e62
add most `staticVariant`s
RobinMalfait Nov 8, 2024
127860e
Revert "do not convert `[&>…]` to `*:…`"
RobinMalfait Nov 8, 2024
7663a79
convert `[&_[data-visible]]` to `**:data-visible`
RobinMalfait Nov 8, 2024
5dac720
rename variables
RobinMalfait Nov 8, 2024
642fa95
compute variant ahead of time
RobinMalfait Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert "do not convert [&>…] to *:…" 
This reverts commit 14b1854.
  • Loading branch information
@RobinMalfait
RobinMalfait committed Nov 9, 2024
commit 127860e2af4ffb4a22d1f861463cfd293c15b08b
13 changes: 2 additions & 11 deletions packages/@tailwindcss-upgrade/src/template/codemods/modernize-arbitrary-values.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,8 @@ test.each([
['[[data-visible]]:flex', 'data-visible:flex'],
['[&[data-visible]]:flex', 'data-visible:flex'],
['[[data-visible]&]:flex', 'data-visible:flex'],

// Keep as-is. Ideally this is converted to `*:data-visible:flex`, but that
// changes the specificity from (0, 2, 0) to (0, 1, 0)
//
// E.g.:
//
// - .\[\&\>\[data-visible\]\]\:flex > [data-visible] (0, 2, 0)
// - [data-visible]:where(.\*\:data-visible\:flex > *) (0, 1, 0)
//
['[&>[data-visible]]:flex', '[&>[data-visible]]:flex'],
['[&_>_[data-visible]]:flex', '[&_>_[data-visible]]:flex'],
['[&>[data-visible]]:flex', '*:data-visible:flex'],
['[&_>_[data-visible]]:flex', '*:data-visible:flex'],

['[&:first-child]:flex', 'first:flex'],
['[&:not(:first-child)]:flex', 'not-first:flex'],
Expand Down
41 changes: 36 additions & 5 deletions packages/@tailwindcss-upgrade/src/template/codemods/modernize-arbitrary-values.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export function modernizeArbitraryValues(
let clone = structuredClone(candidate)
let changed = false

for (let variant of variants(clone)) {
for (let [variant, parent] of variants(clone)) {
// Expecting an arbitrary variant
if (variant.kind !== 'arbitrary') continue

Expand All @@ -26,6 +26,26 @@ export function modernizeArbitraryValues(
// Expecting a single selector node
if (ast.nodes.length !== 1) continue

// Track whether we need to add a `*:` variant
let addChildVariant = false

// Handling a child combinator. E.g.: `[&>[data-visible]]` => `*:data-visible`
if (
// Only top-level, so `has-[&>[data-visible]]` is not supported
parent === null &&
// [&_>_[data-visible]]:flex
// ^ ^ ^^^^^^^^^^^^^^
ast.nodes[0].length === 3 &&
ast.nodes[0].nodes[0].type === 'nesting' &&
ast.nodes[0].nodes[0].value === '&' &&
ast.nodes[0].nodes[1].type === 'combinator' &&
ast.nodes[0].nodes[1].value === '>' &&
ast.nodes[0].nodes[2].type === 'attribute'
) {
ast.nodes[0].nodes = [ast.nodes[0].nodes[2]]
addChildVariant = true
Copy link
Contributor

@thecrypticace thecrypticace Oct 31, 2024

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 •
edited
Loading

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

@thecrypticace thecrypticace Nov 6, 2024

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

@thecrypticace thecrypticace Nov 6, 2024

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

@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.

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

@thecrypticace thecrypticace Nov 6, 2024

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

@adamwathan adamwathan Nov 7, 2024

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

@RobinMalfait RobinMalfait Nov 8, 2024

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

@RobinMalfait RobinMalfait Nov 8, 2024

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(…).

}

// Filter out `&`. E.g.: `&[data-foo]` => `[data-foo]`
let selectorNodes = ast.nodes[0].filter((node) => node.type !== 'nesting')

Expand Down Expand Up @@ -211,6 +231,14 @@ export function modernizeArbitraryValues(
} satisfies Variant)
}
}

if (addChildVariant) {
let idx = clone.variants.indexOf(variant)
if (idx === -1) continue

// Ensure we have the `*:` variant
clone.variants.splice(idx, 1, variant, { kind: 'static', root: '*' })
}
}

return changed ? printCandidate(designSystem, clone) : rawCandidate
Expand All @@ -220,15 +248,18 @@ export function modernizeArbitraryValues(
}

function* variants(candidate: Candidate) {
function* inner(variant: Variant): Iterable<Variant> {
yield variant
function* inner(
variant: Variant,
parent: Extract<Variant, { kind: 'compound' }> | null = null,
): Iterable<[Variant, Extract<Variant, { kind: 'compound' }> | null]> {
yield [variant, parent]

if (variant.kind === 'compound') {
yield* inner(variant.variant)
yield* inner(variant.variant, variant)
}
}

for (let variant of candidate.variants) {
yield* inner(variant)
yield* inner(variant, null)
}
}