From 58edf8e7fb5e60ad51258d6f0769bde78690d0c4 Mon Sep 17 00:00:00 2001 From: Philipp Spiess Date: Fri, 27 Sep 2024 17:52:58 +0200 Subject: [PATCH] Update template migration interface (#14539) This PR lands a quick interface update for template migration with some lessons learned form our existing migrations. Specifically, this version attempts to: - Allow migrations to access the raw candidate. This way we can migrate candidates that _would not parse as valid in v4_. This will help us migrate prefixes in candidates from v3 to v4. - There is no more awkward "return null" if nothing has changed. The return `null` was necessary because we relied on mutating the Variant and since parsing/printing could remove some information, it was not easy to find out wether a candidate needed to be migrated at all. With a string though, we can do this cheaply by returning the `rawCandidate`. - We previously asserted that if `parseCandidate` returns more than one candidate, we only picked the first one. This behavior is now moved into the migrations where we have more context. For now though, we still do not need to worry about this since in all cases, these duplicate candidates would serialize to the same `Candidate`. It is helpful if you only want to run a migration on a specific type of candidate (e.g. if there's a `static` one and a more generic `functional` one). - We need access to the `DesignSystem` inside migrations now to be able to `parseCandidate`s. Opening this up as a separate PR since it can take some time to iron out the edge cases for the individual codemod PRs and I don't want to be rebasing all the time. ## Before ```ts type Migration = (candidate: Candidate) => Candidate | null ``` ## After ```ts type Migration = (designSystem: DesignSystem, rawCandidate: string) => string ``` --- .../src/template/candidates.test.ts | 97 ++++--------------- .../src/template/candidates.ts | 12 +-- .../src/template/codemods/bg-gradient.test.ts | 4 +- .../src/template/codemods/bg-gradient.ts | 23 +++-- .../src/template/codemods/important.test.ts | 22 +++++ .../src/template/codemods/important.ts | 27 ++++++ .../codemods/migrate-important.test.ts | 37 ------- .../template/codemods/migrate-important.ts | 23 ----- .../src/template/migrate.ts | 21 ++-- 9 files changed, 94 insertions(+), 172 deletions(-) create mode 100644 packages/@tailwindcss-upgrade/src/template/codemods/important.test.ts create mode 100644 packages/@tailwindcss-upgrade/src/template/codemods/important.ts delete mode 100644 packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.test.ts delete mode 100644 packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.ts diff --git a/packages/@tailwindcss-upgrade/src/template/candidates.test.ts b/packages/@tailwindcss-upgrade/src/template/candidates.test.ts index 634cbe5e9..e03439ce4 100644 --- a/packages/@tailwindcss-upgrade/src/template/candidates.test.ts +++ b/packages/@tailwindcss-upgrade/src/template/candidates.test.ts @@ -1,6 +1,6 @@ import { __unstable__loadDesignSystem } from '@tailwindcss/node' import { describe, expect, test } from 'vitest' -import { extractCandidates, printCandidate, replaceCandidateInContent } from './candidates' +import { extractRawCandidates, printCandidate, replaceCandidateInContent } from './candidates' let html = String.raw @@ -10,107 +10,40 @@ test('extracts candidates with positions from a template', async () => { ` - let designSystem = await __unstable__loadDesignSystem('@import "tailwindcss";', { base: __dirname, }) - expect(extractCandidates(designSystem, content)).resolves.toMatchInlineSnapshot(` + let candidates = await extractRawCandidates(content) + let validCandidates = candidates.filter( + ({ rawCandidate }) => designSystem.parseCandidate(rawCandidate).length > 0, + ) + + expect(validCandidates).toMatchInlineSnapshot(` [ { - "candidate": { - "important": false, - "kind": "functional", - "modifier": null, - "negative": false, - "raw": "bg-blue-500", - "root": "bg", - "value": { - "fraction": null, - "kind": "named", - "value": "blue-500", - }, - "variants": [], - }, "end": 28, + "rawCandidate": "bg-blue-500", "start": 17, }, { - "candidate": { - "important": false, - "kind": "functional", - "modifier": null, - "negative": false, - "raw": "hover:focus:text-white", - "root": "text", - "value": { - "fraction": null, - "kind": "named", - "value": "white", - }, - "variants": [ - { - "compounds": true, - "kind": "static", - "root": "focus", - }, - { - "compounds": true, - "kind": "static", - "root": "hover", - }, - ], - }, "end": 51, + "rawCandidate": "hover:focus:text-white", "start": 29, }, { - "candidate": { - "important": false, - "kind": "arbitrary", - "modifier": null, - "property": "color", - "raw": "[color:red]", - "value": "red", - "variants": [], - }, "end": 63, + "rawCandidate": "[color:red]", "start": 52, }, { - "candidate": { - "important": false, - "kind": "functional", - "modifier": null, - "negative": false, - "raw": "bg-blue-500", - "root": "bg", - "value": { - "fraction": null, - "kind": "named", - "value": "blue-500", - }, - "variants": [], - }, "end": 98, + "rawCandidate": "bg-blue-500", "start": 87, }, { - "candidate": { - "important": false, - "kind": "functional", - "modifier": null, - "negative": false, - "raw": "text-white", - "root": "text", - "value": { - "fraction": null, - "kind": "named", - "value": "white", - }, - "variants": [], - }, "end": 109, + "rawCandidate": "text-white", "start": 99, }, ] @@ -127,7 +60,11 @@ test('replaces the right positions for a candidate', async () => { base: __dirname, }) - let candidate = (await extractCandidates(designSystem, content))[0] + let candidates = await extractRawCandidates(content) + + let candidate = candidates.find( + ({ rawCandidate }) => designSystem.parseCandidate(rawCandidate).length > 0, + )! expect(replaceCandidateInContent(content, 'flex', candidate.start, candidate.end)) .toMatchInlineSnapshot(` diff --git a/packages/@tailwindcss-upgrade/src/template/candidates.ts b/packages/@tailwindcss-upgrade/src/template/candidates.ts index 19b61d996..a36cc50b0 100644 --- a/packages/@tailwindcss-upgrade/src/template/candidates.ts +++ b/packages/@tailwindcss-upgrade/src/template/candidates.ts @@ -1,20 +1,16 @@ import { Scanner } from '@tailwindcss/oxide' import stringByteSlice from 'string-byte-slice' import type { Candidate, Variant } from '../../../tailwindcss/src/candidate' -import type { DesignSystem } from '../../../tailwindcss/src/design-system' -export async function extractCandidates( - designSystem: DesignSystem, +export async function extractRawCandidates( content: string, -): Promise<{ candidate: Candidate; start: number; end: number }[]> { +): Promise<{ rawCandidate: string; start: number; end: number }[]> { let scanner = new Scanner({}) let result = scanner.getCandidatesWithPositions({ content, extension: 'html' }) - let candidates: { candidate: Candidate; start: number; end: number }[] = [] + let candidates: { rawCandidate: string; start: number; end: number }[] = [] for (let { candidate: rawCandidate, position: start } of result) { - for (let candidate of designSystem.parseCandidate(rawCandidate)) { - candidates.push({ candidate, start, end: start + rawCandidate.length }) - } + candidates.push({ rawCandidate, start, end: start + rawCandidate.length }) } return candidates } diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.test.ts b/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.test.ts index 8ea6572ad..c5811eb81 100644 --- a/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.test.ts +++ b/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.test.ts @@ -1,6 +1,5 @@ import { __unstable__loadDesignSystem } from '@tailwindcss/node' import { expect, test } from 'vitest' -import { printCandidate } from '../candidates' import { bgGradient } from './bg-gradient' test.each([ @@ -19,6 +18,5 @@ test.each([ base: __dirname, }) - let migrated = bgGradient(designSystem.parseCandidate(candidate)[0]!) - expect(migrated ? printCandidate(migrated) : migrated).toEqual(result) + expect(bgGradient(designSystem, candidate)).toEqual(result) }) diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.ts b/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.ts index 8b926a8b2..bf871e24a 100644 --- a/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.ts +++ b/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.ts @@ -1,17 +1,20 @@ -import type { Candidate } from '../../../../tailwindcss/src/candidate' +import type { DesignSystem } from '../../../../tailwindcss/src/design-system' +import { printCandidate } from '../candidates' const DIRECTIONS = ['t', 'tr', 'r', 'br', 'b', 'bl', 'l', 'tl'] -export function bgGradient(candidate: Candidate): Candidate | null { - if (candidate.kind === 'static' && candidate.root.startsWith('bg-gradient-to-')) { - let direction = candidate.root.slice(15) +export function bgGradient(designSystem: DesignSystem, rawCandidate: string): string { + for (let candidate of designSystem.parseCandidate(rawCandidate)) { + if (candidate.kind === 'static' && candidate.root.startsWith('bg-gradient-to-')) { + let direction = candidate.root.slice(15) - if (!DIRECTIONS.includes(direction)) { - return null + if (!DIRECTIONS.includes(direction)) { + continue + } + + candidate.root = `bg-linear-to-${direction}` + return printCandidate(candidate) } - - candidate.root = `bg-linear-to-${direction}` - return candidate } - return null + return rawCandidate } diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/important.test.ts b/packages/@tailwindcss-upgrade/src/template/codemods/important.test.ts new file mode 100644 index 000000000..fd568c500 --- /dev/null +++ b/packages/@tailwindcss-upgrade/src/template/codemods/important.test.ts @@ -0,0 +1,22 @@ +import { __unstable__loadDesignSystem } from '@tailwindcss/node' +import dedent from 'dedent' +import { expect, test } from 'vitest' +import { important } from './important' + +let html = dedent + +test.each([ + ['!flex', 'flex!'], + ['min-[calc(1000px+12em)]:!flex', 'min-[calc(1000px_+_12em)]:flex!'], + ['md:!block', 'md:block!'], + + // Does not change non-important candidates + ['bg-blue-500', 'bg-blue-500'], + ['min-[calc(1000px+12em)]:flex', 'min-[calc(1000px+12em)]:flex'], +])('%s => %s', async (candidate, result) => { + let designSystem = await __unstable__loadDesignSystem('@import "tailwindcss";', { + base: __dirname, + }) + + expect(important(designSystem, candidate)).toEqual(result) +}) diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/important.ts b/packages/@tailwindcss-upgrade/src/template/codemods/important.ts new file mode 100644 index 000000000..d6c6fc2db --- /dev/null +++ b/packages/@tailwindcss-upgrade/src/template/codemods/important.ts @@ -0,0 +1,27 @@ +import type { DesignSystem } from '../../../../tailwindcss/src/design-system' +import { printCandidate } from '../candidates' + +// In v3 the important modifier `!` sits in front of the utility itself, not +// before any of the variants. In v4, we want it to be at the end of the utility +// so that it's always in the same location regardless of whether you used +// variants or not. +// +// So this: +// +// !flex md:!block +// +// Should turn into: +// +// flex! md:block! +export function important(designSystem: DesignSystem, rawCandidate: string): string { + for (let candidate of designSystem.parseCandidate(rawCandidate)) { + if (candidate.important && candidate.raw[candidate.raw.length - 1] !== '!') { + // The printCandidate function will already put the exclamation mark in + // the right place, so we just need to mark this candidate as requiring a + // migration. + return printCandidate(candidate) + } + } + + return rawCandidate +} diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.test.ts b/packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.test.ts deleted file mode 100644 index 0755dd41a..000000000 --- a/packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.test.ts +++ /dev/null @@ -1,37 +0,0 @@ -import { __unstable__loadDesignSystem } from '@tailwindcss/node' -import dedent from 'dedent' -import { expect, test } from 'vitest' -import migrate from '../migrate' -import { migrateImportant } from './migrate-important' - -let html = dedent - -test('applies the migration', async () => { - let content = html` -
- -
- ` - - let designSystem = await __unstable__loadDesignSystem('@import "tailwindcss";', { - base: __dirname, - }) - - expect(migrate(designSystem, content, [migrateImportant])).resolves.toMatchInlineSnapshot(` - "
- -
" - `) -}) - -test('does not migrate if the exclamation mark is already at the end', async () => { - let content = html`
` - - let designSystem = await __unstable__loadDesignSystem('@import "tailwindcss";', { - base: __dirname, - }) - - expect(migrate(designSystem, content, [migrateImportant])).resolves.toMatchInlineSnapshot(` - "
" - `) -}) diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.ts b/packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.ts deleted file mode 100644 index 44a23a1cb..000000000 --- a/packages/@tailwindcss-upgrade/src/template/codemods/migrate-important.ts +++ /dev/null @@ -1,23 +0,0 @@ -import type { Candidate } from '../../../../tailwindcss/src/candidate' - -// In v3 the important modifier `!` sits in front of the utility itself, not -// before any of the variants. In v4, we want it to be at the end of the utility -// so that it's always in the same location regardless of whether you used -// variants or not. -// -// So this: -// -// !flex md:!block -// -// Should turn into: -// -// flex! md:block! -export function migrateImportant(candidate: Candidate): Candidate | null { - if (candidate.important && candidate.raw[candidate.raw.length - 1] !== '!') { - // The printCandidate function will already put the exclamation mark in the - // right place, so we just need to mark this candidate as requiring a - // migration. - return candidate - } - return null -} diff --git a/packages/@tailwindcss-upgrade/src/template/migrate.ts b/packages/@tailwindcss-upgrade/src/template/migrate.ts index f1228cc81..44f2c6215 100644 --- a/packages/@tailwindcss-upgrade/src/template/migrate.ts +++ b/packages/@tailwindcss-upgrade/src/template/migrate.ts @@ -1,36 +1,35 @@ import fs from 'node:fs/promises' import path from 'node:path' -import type { Candidate } from '../../../tailwindcss/src/candidate' import type { DesignSystem } from '../../../tailwindcss/src/design-system' -import { extractCandidates, printCandidate, replaceCandidateInContent } from './candidates' +import { extractRawCandidates, replaceCandidateInContent } from './candidates' import { bgGradient } from './codemods/bg-gradient' -import { migrateImportant } from './codemods/migrate-important' +import { important } from './codemods/important' -export type Migration = (candidate: Candidate) => Candidate | null +export type Migration = (designSystem: DesignSystem, rawCandidate: string) => string export default async function migrateContents( designSystem: DesignSystem, contents: string, - migrations: Migration[] = [migrateImportant, bgGradient], + migrations: Migration[] = [important, bgGradient], ): Promise { - let candidates = await extractCandidates(designSystem, contents) + let candidates = await extractRawCandidates(contents) // Sort candidates by starting position desc candidates.sort((a, z) => z.start - a.start) let output = contents - for (let { candidate, start, end } of candidates) { + for (let { rawCandidate, start, end } of candidates) { let needsMigration = false for (let migration of migrations) { - let migrated = migration(candidate) - if (migrated) { - candidate = migrated + let candidate = migration(designSystem, rawCandidate) + if (rawCandidate !== candidate) { + rawCandidate = candidate needsMigration = true } } if (needsMigration) { - output = replaceCandidateInContent(output, printCandidate(candidate), start, end) + output = replaceCandidateInContent(output, rawCandidate, start, end) } }