Skip to content
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.

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

Ensure layer(…) on @import is only removed when @utility is present #14783

Merged
merged 4 commits into from
Oct 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
9 changes: 5 additions & 4 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand All @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this down to be consistent with other changelog entries


- 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

Expand Down
86 changes: 85 additions & 1 deletion integrations/upgrade/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it important that components comes last here? If that's not a crucial part of the test it seems better to me to structure things closer to how they would be in a real project.

`,
'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;
}
"
`)
},
)
Original file line number Diff line number Diff line change
Expand Up @@ -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`
Expand Down
49 changes: 15 additions & 34 deletions packages/@tailwindcss-upgrade/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

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
}
}
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

// Only remove the `layer(…)` next to the import, if any of the children
// contains an `@utility`. Otherwise the `@utility` will not be top-level.
Comment on lines +161 to +162
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Only remove the `layer(…)` next to the import, if any of the children
// contains an `@utility`. Otherwise the `@utility` will not be top-level.
// Only remove the `layer(…)` next to the import if any of the children
// contain `@utility`. Otherwise `@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()
}
}

Expand Down
23 changes: 23 additions & 0 deletions packages/@tailwindcss-upgrade/src/stylesheet.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down