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

RobinMalfait · 2024-10-24T16:22:01Z

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:

tailwindcss/integrations/upgrade/index.test.ts

Lines 2076 to 2108 in 96e8908

    
                 '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 { 
        
                   } 
        
                 `,

Output:

tailwindcss/integrations/upgrade/index.test.ts

Lines 2116 to 2126 in 96e8908

    
                 --- ./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);

RobinMalfait · 2024-10-24T17:48:37Z

CHANGELOG.md

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


Moved this down to be consistent with other changelog entries

adamwathan · 2024-10-24T18:29:25Z

packages/@tailwindcss-upgrade/src/index.ts

+        // Only remove the `layer(…)` next to the import, if any of the children
+        // contains an `@utility`. Otherwise the `@utility` will not be top-level.


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.

adamwathan · 2024-10-24T18:31:34Z

integrations/upgrade/index.test.ts

+        /* Expecting a layer */
+        @import './my-other.css';
+
+        @import 'tailwindcss/components';


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.

adamwathan

Going to merge as-is so we can tag a release since this will already be an improvement over what's there but would like to come back and address the feedback here too before we forget 👍

This PR is a continuation of #14783 to handle the feedback on that PR. 1. Update the test to be more realistic 2. Updated the comment --------- Co-authored-by: Adam Wathan <[email protected]>

RobinMalfait force-pushed the robin/ensure_layer_on_import_is_correct branch 5 times, most recently from 380c128 to 1342dc8 Compare October 24, 2024 17:33

RobinMalfait added 2 commits October 24, 2024 19:33

ensure layer(…) on @import is correct

86f3b8c

add test

4e5b48e

RobinMalfait force-pushed the robin/ensure_layer_on_import_is_correct branch from 1342dc8 to 4e5b48e Compare October 24, 2024 17:33

update changelog

fbc1834

RobinMalfait changed the title ~~Ensure layer(…) on @import is correct~~ Ensure layer(…) on @import is only removed when @utility is present Oct 24, 2024

update changelog

72b4b91

RobinMalfait marked this pull request as ready for review October 24, 2024 17:43

RobinMalfait requested a review from a team as a code owner October 24, 2024 17:43

RobinMalfait commented Oct 24, 2024

View reviewed changes

adamwathan reviewed Oct 24, 2024

View reviewed changes

adamwathan approved these changes Oct 24, 2024

View reviewed changes

adamwathan merged commit 430836f into next Oct 24, 2024
1 check passed

adamwathan deleted the robin/ensure_layer_on_import_is_correct branch October 24, 2024 18:33

RobinMalfait mentioned this pull request Oct 24, 2024

Handle feedback from #14783 #14788

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

RobinMalfait commented Oct 24, 2024 •

edited

Loading

RobinMalfait Oct 24, 2024

adamwathan Oct 24, 2024

adamwathan Oct 24, 2024

adamwathan left a comment

	'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 {
	}
	`,

	--- ./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);

		// Only remove the `layer(…)` next to the import, if any of the children
		// contains an `@utility`. Otherwise the `@utility` will not be top-level.

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

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

Conversation

RobinMalfait commented Oct 24, 2024 • edited Loading

Test plan:

RobinMalfait Oct 24, 2024

Choose a reason for hiding this comment

adamwathan Oct 24, 2024

Choose a reason for hiding this comment

adamwathan Oct 24, 2024

Choose a reason for hiding this comment

adamwathan left a comment

Choose a reason for hiding this comment

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

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

RobinMalfait commented Oct 24, 2024 •

edited

Loading