From 430836f6513e7b312ca97dad925bbfa91f8cd987 Mon Sep 17 00:00:00 2001 From: Robin Malfait Date: Thu, 24 Oct 2024 20:33:10 +0200 Subject: [PATCH] =?UTF-8?q?Ensure=20`layer(=E2=80=A6)`=20on=20`@import`=20?= =?UTF-8?q?is=20only=20removed=20when=20`@utility`=20is=20present=20(#1478?= =?UTF-8?q?3)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR fixes an issue where `layer(…)` next to imports were removed where they shouldn't have been removed. The issue exists if _any_ of the `@import` nodes in a file contains `@utility`, if that's the case then we removed the `layer(…)` next to _all_ `@import` nodes. Before we were checking if the current sheet contained `@utility` or in any of its children (sub-`@import` nodes). This fixes that by looping over the `@import` nodes in the current sheet, and looking for the `@utility` in the associated/imported file. This way we update each node individually. Test plan: --- Added a dedicated integration test to make sure all codemods together result in the correct result. Input: https://github.com/tailwindlabs/tailwindcss/blob/96e890837809487fecdd713695294cb9961cc1d6/integrations/upgrade/index.test.ts#L2076-L2108 Output: https://github.com/tailwindlabs/tailwindcss/blob/96e890837809487fecdd713695294cb9961cc1d6/integrations/upgrade/index.test.ts#L2116-L2126 --- CHANGELOG.md | 9 +- integrations/upgrade/index.test.ts | 86 ++++++++++++++++++- .../codemods/migrate-missing-layers.test.ts | 16 ++++ packages/@tailwindcss-upgrade/src/index.ts | 47 +++------- .../@tailwindcss-upgrade/src/stylesheet.ts | 23 +++++ 5 files changed, 143 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 352bfa4fb..11c868d14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,10 +12,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Added `not-*` versions of all builtin media query and supports variants ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743)) - Improved support for custom variants with `group-*`, `peer-*`, `has-*`, and `not-*` ([#14743](https://github.com/tailwindlabs/tailwindcss/pull/14743)) -### Changed - -- Don't convert underscores in the first argument to `var()` and `theme()` to spaces ([#14776](https://github.com/tailwindlabs/tailwindcss/pull/14776), [#14781](https://github.com/tailwindlabs/tailwindcss/pull/14781)) - ### Fixed - Ensure individual logical property utilities are sorted later than left/right pair utilities ([#14777](https://github.com/tailwindlabs/tailwindcss/pull/14777)) @@ -26,6 +22,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - _Upgrade (experimental)_: Ensure `@import` statements for relative CSS files are actually migrated to use relative path syntax ([#14769](https://github.com/tailwindlabs/tailwindcss/pull/14769)) - _Upgrade (experimental)_: Only generate Preflight compatibility styles when Preflight is used ([#14773](https://github.com/tailwindlabs/tailwindcss/pull/14773)) - _Upgrade (experimental)_: Don't escape underscores when printing theme values migrated to CSS variables in arbitrary values (e.g. `m-[var(--spacing-1_5)]` instead of `m-[var(--spacing-1\_5)]`) ([#14778](https://github.com/tailwindlabs/tailwindcss/pull/14778)) +- _Upgrade (experimental)_: Ensure `layer(…)` on `@import` is only removed when `@utility` is present ([#14783](https://github.com/tailwindlabs/tailwindcss/pull/14783)) + +### Changed + +- Don't convert underscores in the first argument to `var()` and `theme()` to spaces ([#14776](https://github.com/tailwindlabs/tailwindcss/pull/14776), [#14781](https://github.com/tailwindlabs/tailwindcss/pull/14781)) ## [4.0.0-alpha.29] - 2024-10-23 diff --git a/integrations/upgrade/index.test.ts b/integrations/upgrade/index.test.ts index 9d1e4230e..3680e8ae3 100644 --- a/integrations/upgrade/index.test.ts +++ b/integrations/upgrade/index.test.ts @@ -1024,7 +1024,7 @@ test( @import './a.1.css' layer(utilities); @import './a.1.utilities.1.css'; @import './b.1.css'; - @import './c.1.css'; + @import './c.1.css' layer(utilities); @import './c.1.utilities.css'; @import './d.1.css'; @@ -1545,3 +1545,87 @@ test( `) }, ) + +test( + 'that it attaches the correct layers to the imported files', + { + fs: { + 'package.json': json` + { + "dependencies": { + "tailwindcss": "workspace:^", + "@tailwindcss/upgrade": "workspace:^" + } + } + `, + 'tailwind.config.js': js`module.exports = {}`, + 'src/index.css': css` + @import 'tailwindcss/utilities'; + + /* No layer expected */ + @import './my-components.css'; + + /* No layer expected */ + @import './my-utilities.css'; + + /* Expecting a layer */ + @import './my-other.css'; + + @import 'tailwindcss/components'; + `, + 'src/my-components.css': css` + @layer components { + .foo { + color: red; + } + } + `, + 'src/my-utilities.css': css` + @layer utilities { + .css { + color: red; + } + } + `, + 'src/my-other.css': css` + /* All my fonts! */ + @font-face { + } + `, + }, + }, + async ({ fs, exec }) => { + await exec('npx @tailwindcss/upgrade --force') + + expect(await fs.dumpFiles('./src/**/*.css')).toMatchInlineSnapshot(` + " + --- ./src/index.css --- + @import 'tailwindcss/utilities' layer(utilities); + + /* No layer expected */ + @import './my-components.css'; + + /* No layer expected */ + @import './my-utilities.css'; + + /* Expecting a layer */ + @import './my-other.css' layer(utilities); + + --- ./src/my-components.css --- + @utility foo { + color: red; + } + + --- ./src/my-other.css --- + /* All my fonts! */ + @font-face { + } + + --- ./src/my-utilities.css --- + @utility css { + color: red; + } + " + `) + }, +) diff --git a/packages/@tailwindcss-upgrade/src/codemods/migrate-missing-layers.test.ts b/packages/@tailwindcss-upgrade/src/codemods/migrate-missing-layers.test.ts index 7dec9bcda..d2782863a 100644 --- a/packages/@tailwindcss-upgrade/src/codemods/migrate-missing-layers.test.ts +++ b/packages/@tailwindcss-upgrade/src/codemods/migrate-missing-layers.test.ts @@ -22,6 +22,22 @@ it('should not migrate already migrated `@import` at-rules', async () => { ).toMatchInlineSnapshot(`"@import 'tailwindcss';"`) }) +it('should add missing `layer(…)` to imported files', async () => { + expect( + await migrate(css` + @import 'tailwindcss/utilities'; /* Expected no layer */ + @import './foo.css'; /* Expected layer(utilities) */ + @import './bar.css'; /* Expected layer(utilities) */ + @import 'tailwindcss/components'; /* Expected no layer */ + `), + ).toMatchInlineSnapshot(` + "@import 'tailwindcss/utilities'; /* Expected no layer */ + @import './foo.css' layer(utilities); /* Expected layer(utilities) */ + @import './bar.css' layer(utilities); /* Expected layer(utilities) */ + @import 'tailwindcss/components'; /* Expected no layer */" + `) +}) + it('should not migrate anything if no `@tailwind` directives (or imports) are found', async () => { expect( await migrate(css` diff --git a/packages/@tailwindcss-upgrade/src/index.ts b/packages/@tailwindcss-upgrade/src/index.ts index 17a1041c8..b2f211b45 100644 --- a/packages/@tailwindcss-upgrade/src/index.ts +++ b/packages/@tailwindcss-upgrade/src/index.ts @@ -151,42 +151,23 @@ async function run() { // Cleanup `@import "…" layer(utilities)` for (let sheet of stylesheets) { - // If the `@import` contains an injected `layer(…)` we need to remove it - if (!Array.from(sheet.importRules).some((node) => node.raws.tailwind_injected_layer)) { - continue - } + for (let importRule of sheet.importRules) { + if (!importRule.raws.tailwind_injected_layer) continue + let importedSheet = stylesheets.find( + (sheet) => sheet.id === importRule.raws.tailwind_destination_sheet_id, + ) + if (!importedSheet) continue - let hasAtUtility = false - - // Only remove the `layer(…)` next to the import, if any of the children - // contains an `@utility`. Otherwise the `@utility` will not be top-level. - { - sheet.root.walkAtRules('utility', () => { - hasAtUtility = true - return false - }) - - if (!hasAtUtility) { - for (let child of sheet.descendants()) { - child.root.walkAtRules('utility', () => { - hasAtUtility = true - return false - }) - - if (hasAtUtility) { - break - } - } + // Only remove the `layer(…)` next to the import, if any of the children + // contains an `@utility`. Otherwise the `@utility` will not be top-level. + if ( + !importedSheet.containsRule((node) => node.type === 'atrule' && node.name === 'utility') + ) { + continue } - } - // No `@utility` found, we can keep the `layer(…)` next to the import - if (!hasAtUtility) continue - - for (let importNode of sheet.importRules) { - if (importNode.raws.tailwind_injected_layer) { - importNode.params = importNode.params.replace(/ layer\([^)]+\)/, '').trim() - } + // Make sure to remove the `layer(…)` from the `@import` at-rule + importRule.params = importRule.params.replace(/ layer\([^)]+\)/, '').trim() } } diff --git a/packages/@tailwindcss-upgrade/src/stylesheet.ts b/packages/@tailwindcss-upgrade/src/stylesheet.ts index 1b8fa841e..1bea7ad76 100644 --- a/packages/@tailwindcss-upgrade/src/stylesheet.ts +++ b/packages/@tailwindcss-upgrade/src/stylesheet.ts @@ -219,6 +219,29 @@ export class Stylesheet { return { convertiblePaths, nonConvertiblePaths } } + containsRule(cb: (rule: postcss.AnyNode) => boolean) { + let contains = false + + this.root.walk((rule) => { + if (cb(rule)) { + contains = true + return false + } + }) + + if (contains) { + return true + } + + for (let child of this.children) { + if (child.item.containsRule(cb)) { + return true + } + } + + return false + } + [util.inspect.custom]() { return { ...this,