Enable SCSS important @apply migration test#20106
Conversation
The upgrade codemod already supports Sass/SCSS interpolation syntax when migrating @apply ... #{!important} to postfix important utilities, but the corresponding test was still skipped with a stale TODO. Enable the test so the behavior is covered against regressions.
Confidence Score: 5/5The change is limited to a test file, removes a skip, and adds no production code — safe to merge. Only the test file changes. The new migrateRoot helper correctly targets the plugin's OnceExit handler, the manual AST manipulation accurately reproduces the SCSS interpolation string the production code already handles, and the extracted loadDesignSystem helper reduces duplication without altering any behavior. No files require special attention. Reviews (1): Last reviewed commit: "Enable SCSS important @apply migration t..." | Re-trigger Greptile |
WalkthroughThis PR refactors the test harness for the 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts (1)
35-49: ⚡ Quick winMemoize design-system loading to avoid repeated expensive test setup.
loadDesignSystem()currently rebuilds the design system on every call frommigrate()/migrateRoot(). Caching the promise keeps behavior the same and speeds up this test file noticeably.Proposed diff
+let designSystemPromise: ReturnType<typeof __unstable__loadDesignSystem> | null = null + async function loadDesignSystem() { - let designSystem = await __unstable__loadDesignSystem( - css` - `@import` 'tailwindcss'; - - /* TODO(perf): Only here to speed up the tests */ - `@theme` { - --*: initial; - } - `, - { base: __dirname }, - ) - - return designSystem + return (designSystemPromise ??= __unstable__loadDesignSystem( + css` + `@import` 'tailwindcss'; + + /* TODO(perf): Only here to speed up the tests */ + `@theme` { + --*: initial; + } + `, + { base: __dirname }, + )) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/`@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts around lines 35 - 49, The loadDesignSystem() helper rebuilds the design system on every invocation; memoize the creation promise so subsequent calls reuse the same design system. Add a module-scoped variable (e.g., cachedDesignSystemPromise) and have loadDesignSystem return that promise if set; otherwise call __unstable__loadDesignSystem(css`...`, { base: __dirname }), store the returned promise in cachedDesignSystemPromise, and return it. Ensure the behavior and returned value remain identical to callers of migrate() and migrateRoot().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/`@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts:
- Around line 35-49: The loadDesignSystem() helper rebuilds the design system on
every invocation; memoize the creation promise so subsequent calls reuse the
same design system. Add a module-scoped variable (e.g.,
cachedDesignSystemPromise) and have loadDesignSystem return that promise if set;
otherwise call __unstable__loadDesignSystem(css`...`, { base: __dirname }),
store the returned promise in cachedDesignSystemPromise, and return it. Ensure
the behavior and returned value remain identical to callers of migrate() and
migrateRoot().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: bf841e6c-604d-4d6e-b277-fbf37588f4a6
📒 Files selected for processing (1)
packages/@tailwindcss-upgrade/src/codemods/css/migrate-at-apply.test.ts
This PR cleans up some old stale test that has been skipped from the
beginning.
While this test wants to prove that migrating from Tailwind CSS v3 to
Tailwind CSS v4 can handle the `#{!important}` SCSS notation, it doesn't
prove that we can migrate an entire SCSS project. SCSS has much more
special syntax and we never supported that.
Let's get rid of this test that's doing nothing at the moment.
Especially since we don't want to migrate SCSS projects. Enabling this
might result in the false sense that we _do_ support SCSS migrations
which is not the case.
Closes: #20106
|
Hey! I appreciate the PR, but the test itself is stale and I removed it via #20117. At some point during the development of the Tailwind CSS v3 → Tailwind CSS v4 migration tool, we were thinking about SCSS projects, but we never fully wanted to support it because there is so much more non-CSS syntax in there. Enabling this might result in the false sense that we do support migrating SCSS/SASS projects and that's not something we want to do. Thanks! |
The upgrade codemod already supports Sass/SCSS interpolation syntax when migrating @apply ... #{!important} to postfix important utilities, but the corresponding test was still skipped with a stale TODO.