Fix template migration issues (#14600)

This PR fixes two issues we found when testing the candidate codemodes: 1. Sometimes, core would emit the same candidate twice. This would result into rewriting a range multiple times, without realizing that this change might already be applied, causing it to swallow or duplicate some bytes. 2. The codemods were mutating the `Candidate` object, however since the `Candidate` parsing is _cached_ in core, it would sometimes return the same instance. This is an issue especially since we monkey patch the prefix to `null` when migrating prefixed candidates. This means that a candidate would be cached that would be _invalid relative to the real design system_. We fixed this by making sure the mutations would only be applied to clones of the `Candidate` and I changed the `DesignSystem` API to return `ReadOnly<T>` versions of these candidates. A better solution would maybe be to disable the cache at all but this requires broader changes in Core.
tailwindlabs · Oct 8, 2024 · 1467dab · 1467dab
1 parent fdb90ae
commit 1467dab
Show file tree

Hide file tree

Showing 12 changed files with 142 additions and 24 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -17,7 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - Don’t crash when scanning a candidate equal to the configured prefix ([#14588](https://github.com/tailwindlabs/tailwindcss/pull/14588))
 - Ensure there's always a space before `!important` when stringifying CSS ([#14611](https://github.com/tailwindlabs/tailwindcss/pull/14611))
-- _Experimental_: Ensure CSS before a layer stays unlayered when running codemods ([#14596](https://github.com/tailwindlabs/tailwindcss/pull/14596))
+- _Upgrade (experimental)_: Ensure CSS before a layer stays unlayered when running codemods ([#14596](https://github.com/tailwindlabs/tailwindcss/pull/14596))
+- _Upgrade (experimental)_: Resolve issues where some prefixed candidates were not properly migrated ([#14600](https://github.com/tailwindlabs/tailwindcss/pull/14600))
 
 ## [4.0.0-alpha.26] - 2024-10-03
 

diff --git a/crates/oxide/src/parser.rs b/crates/oxide/src/parser.rs
@@ -861,13 +861,17 @@ impl<'a> Extractor<'a> {
                 ParseAction::SingleCandidate(candidate)
             }
             Bracketing::Included(sliceable) | Bracketing::Wrapped(sliceable) => {
-                let parts = vec![candidate, sliceable];
-                let parts = parts
-                    .into_iter()
-                    .filter(|v| !v.is_empty())
-                    .collect::<Vec<_>>();
+                if candidate == sliceable {
+                    ParseAction::SingleCandidate(candidate)
+                } else {
+                    let parts = vec![candidate, sliceable];
+                    let parts = parts
+                        .into_iter()
+                        .filter(|v| !v.is_empty())
+                        .collect::<Vec<_>>();
 
-                ParseAction::MultipleCandidates(parts)
+                    ParseAction::MultipleCandidates(parts)
+                }
             }
         }
     }
@@ -1185,7 +1189,7 @@ mod test {
     fn bad_003() {
         // TODO: This seems… wrong
         let candidates = run(r"[𕤵:]", false);
-        assert_eq!(candidates, vec!["𕤵", "𕤵:"]);
+        assert_eq!(candidates, vec!["𕤵", "𕤵:",]);
     }
 
     #[test]
@@ -1436,4 +1440,15 @@ mod test {
             .unwrap();
         assert_eq!(result, Some("[.foo_&]:px-[0]"));
     }
+
+    #[test]
+    fn does_not_emit_the_same_slice_multiple_times() {
+        let candidates: Vec<_> =
+            Extractor::with_positions("<div class=\"flex\"></div>".as_bytes(), Default::default())
+                .into_iter()
+                .map(|(s, p)| unsafe { (std::str::from_utf8_unchecked(s), p) })
+                .collect();
+
+        assert_eq!(candidates, vec![("div", 1), ("class", 5), ("flex", 12),]);
+    }
 }
diff --git a/integrations/upgrade/index.test.ts b/integrations/upgrade/index.test.ts
@@ -264,3 +264,91 @@ test(
     )
   },
 )
+
+test(
+  `migrates prefixes even if other files have unprefixed versions of the candidate`,
+  {
+    fs: {
+      'package.json': json`
+        {
+          "dependencies": {
+            "@tailwindcss/upgrade": "workspace:^"
+          }
+        }
+      `,
+      'tailwind.config.js': js`
+        /** @type {import('tailwindcss').Config} */
+        module.exports = {
+          content: ['./src/**/*.{html,js}'],
+          prefix: 'tw__',
+        }
+      `,
+      'src/index.html': html`
+        <div class="flex"></div>
+      `,
+      'src/other.html': html`
+        <div class="tw__flex"></div>
+      `,
+      'src/input.css': css`
+        @tailwind base;
+        @tailwind components;
+        @tailwind utilities;
+      `,
+    },
+  },
+  async ({ exec, fs }) => {
+    await exec('npx @tailwindcss/upgrade -c tailwind.config.js')
+
+    await fs.expectFileToContain('src/index.html', html`
+        <div class="flex"></div>
+      `)
+    await fs.expectFileToContain('src/other.html', html`
+        <div class="tw:flex"></div>
+      `)
+  },
+)
+
+test(
+  `prefixed variants do not cause their unprefixed counterparts to be valid`,
+  {
+    fs: {
+      'package.json': json`
+        {
+          "dependencies": {
+            "@tailwindcss/upgrade": "workspace:^"
+          }
+        }
+      `,
+      'tailwind.config.js': js`
+        /** @type {import('tailwindcss').Config} */
+        module.exports = {
+          content: ['./src/**/*.{html,js}'],
+          prefix: 'tw__',
+        }
+      `,
+      'src/index.html': html`
+        <div class="tw__bg-gradient-to-t"></div>
+      `,
+      'src/other.html': html`
+        <div class="bg-gradient-to-t"></div>
+      `,
+    },
+  },
+  async ({ exec, fs }) => {
+    await exec('npx @tailwindcss/upgrade -c tailwind.config.js')
+
+    await fs.expectFileToContain(
+      'src/index.html',
+      html`
+        <div class="tw:bg-linear-to-t"></div>
+      `,
+    )
+
+    await fs.expectFileToContain(
+      'src/other.html',
+      html`
+        <div class="bg-gradient-to-t"></div>
+      `,
+    )
+  },
+)
diff --git a/integrations/utils.ts b/integrations/utils.ts
@@ -74,7 +74,7 @@ export function test(
 ) {
   return (only || (!process.env.CI && debug) ? defaultTest.only : defaultTest)(
     name,
-    { timeout: TEST_TIMEOUT, retry: 3 },
+    { timeout: TEST_TIMEOUT, retry: debug ? 0 : 3 },
     async (options) => {
       let rootDir = debug ? path.join(REPO_ROOT, '.debug') : TMP_ROOT
       await fs.mkdir(rootDir, { recursive: true })

diff --git a/packages/@tailwindcss-upgrade/src/template/candidates.test.ts b/packages/@tailwindcss-upgrade/src/template/candidates.test.ts
@@ -14,7 +14,7 @@ test('extracts candidates with positions from a template', async () => {
     base: __dirname,
   })
 
-  let candidates = await extractRawCandidates(content)
+  let candidates = await extractRawCandidates(content, 'html')
   let validCandidates = candidates.filter(
     ({ rawCandidate }) => designSystem.parseCandidate(rawCandidate).length > 0,
   )
@@ -60,7 +60,7 @@ test('replaces the right positions for a candidate', async () => {
     base: __dirname,
   })
 
-  let candidates = await extractRawCandidates(content)
+  let candidates = await extractRawCandidates(content, 'html')
 
   let candidate = candidates.find(
     ({ rawCandidate }) => designSystem.parseCandidate(rawCandidate).length > 0,

diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.ts b/packages/@tailwindcss-upgrade/src/template/codemods/automatic-var-injection.ts
@@ -1,6 +1,6 @@
 import type { Config } from 'tailwindcss'
 import { walk, WalkAction } from '../../../../tailwindcss/src/ast'
-import type { Candidate, Variant } from '../../../../tailwindcss/src/candidate'
+import { type Candidate, type Variant } from '../../../../tailwindcss/src/candidate'
 import type { DesignSystem } from '../../../../tailwindcss/src/design-system'
 import { printCandidate } from '../candidates'
 
@@ -9,7 +9,11 @@ export function automaticVarInjection(
   _userConfig: Config,
   rawCandidate: string,
 ): string {
-  for (let candidate of designSystem.parseCandidate(rawCandidate)) {
+  for (let readonlyCandidate of designSystem.parseCandidate(rawCandidate)) {
+    // The below logic makes extended use of mutation. Since candidates in the
+    // DesignSystem are cached, we can't mutate them directly.
+    let candidate = structuredClone(readonlyCandidate) as Candidate
+
     let didChange = false
 
     // Add `var(…)` in modifier position, e.g.:

diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.ts b/packages/@tailwindcss-upgrade/src/template/codemods/bg-gradient.ts
@@ -17,8 +17,10 @@ export function bgGradient(
         continue
       }
 
-      candidate.root = `bg-linear-to-${direction}`
-      return printCandidate(designSystem, candidate)
+      return printCandidate(designSystem, {
+        ...candidate,
+        root: `bg-linear-to-${direction}`,
+      })
     }
   }
   return rawCandidate

diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/important.ts b/packages/@tailwindcss-upgrade/src/template/codemods/important.ts
@@ -1,4 +1,5 @@
 import type { Config } from 'tailwindcss'
+import { parseCandidate } from '../../../../tailwindcss/src/candidate'
 import type { DesignSystem } from '../../../../tailwindcss/src/design-system'
 import { printCandidate } from '../candidates'
 
@@ -19,7 +20,7 @@ export function important(
   _userConfig: Config,
   rawCandidate: string,
 ): string {
-  for (let candidate of designSystem.parseCandidate(rawCandidate)) {
+  for (let candidate of parseCandidate(rawCandidate, designSystem)) {
     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

diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/prefix.ts b/packages/@tailwindcss-upgrade/src/template/codemods/prefix.ts
@@ -1,5 +1,5 @@
 import type { Config } from 'tailwindcss'
-import type { Candidate } from '../../../../tailwindcss/src/candidate'
+import { parseCandidate, type Candidate } from '../../../../tailwindcss/src/candidate'
 import type { DesignSystem } from '../../../../tailwindcss/src/design-system'
 import { segment } from '../../../../tailwindcss/src/utils/segment'
 import { printCandidate } from '../candidates'
@@ -24,7 +24,10 @@ export function prefix(
     let unprefixedCandidate =
       rawCandidate.slice(0, v3Base.start) + v3Base.base + rawCandidate.slice(v3Base.end)
 
-    let candidates = designSystem.parseCandidate(unprefixedCandidate)
+    // Note: This is not a valid candidate in the original DesignSystem, so we
+    // can not use the `DesignSystem#parseCandidate` API here or otherwise this
+    // invalid candidate will be cached.
+    let candidates = [...parseCandidate(unprefixedCandidate, designSystem)]
     if (candidates.length > 0) {
       candidate = candidates[0]
     }

diff --git a/packages/@tailwindcss-upgrade/src/template/codemods/variant-order.ts b/packages/@tailwindcss-upgrade/src/template/codemods/variant-order.ts
@@ -1,6 +1,6 @@
 import type { Config } from 'tailwindcss'
 import { walk, type AstNode } from '../../../../tailwindcss/src/ast'
-import type { Variant } from '../../../../tailwindcss/src/candidate'
+import { type Variant } from '../../../../tailwindcss/src/candidate'
 import type { DesignSystem } from '../../../../tailwindcss/src/design-system'
 import { printCandidate } from '../candidates'
 

diff --git a/packages/@tailwindcss-upgrade/src/template/migrate.ts b/packages/@tailwindcss-upgrade/src/template/migrate.ts
@@ -1,5 +1,5 @@
 import fs from 'node:fs/promises'
-import path from 'node:path'
+import path, { extname } from 'node:path'
 import type { Config } from 'tailwindcss'
 import type { DesignSystem } from '../../../tailwindcss/src/design-system'
 import { extractRawCandidates, replaceCandidateInContent } from './candidates'
@@ -38,8 +38,9 @@ export default async function migrateContents(
   designSystem: DesignSystem,
   userConfig: Config,
   contents: string,
+  extension: string,
 ): Promise<string> {
-  let candidates = await extractRawCandidates(contents)
+  let candidates = await extractRawCandidates(contents, extension)
 
   // Sort candidates by starting position desc
   candidates.sort((a, z) => z.start - a.start)
@@ -60,5 +61,8 @@ export async function migrate(designSystem: DesignSystem, userConfig: Config, fi
   let fullPath = path.resolve(process.cwd(), file)
   let contents = await fs.readFile(fullPath, 'utf-8')
 
-  await fs.writeFile(fullPath, await migrateContents(designSystem, userConfig, contents))
+  await fs.writeFile(
+    fullPath,
+    await migrateContents(designSystem, userConfig, contents, extname(file)),
+  )
 }
diff --git a/packages/tailwindcss/src/design-system.ts b/packages/tailwindcss/src/design-system.ts
@@ -22,8 +22,8 @@ export type DesignSystem = {
   getClassList(): ClassEntry[]
   getVariants(): VariantEntry[]
 
-  parseCandidate(candidate: string): Candidate[]
-  parseVariant(variant: string): Variant | null
+  parseCandidate(candidate: string): Readonly<Candidate>[]
+  parseVariant(variant: string): Readonly<Variant> | null
   compileAstNodes(candidate: Candidate): ReturnType<typeof compileAstNodes>
 
   getVariantOrder(): Map<Variant, number>