CSS codemod: inject @import in a more expected location
#14536
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RobinMalfait
changed the title
@improt in a more expected location@import in a more expected location
RobinMalfait
commented
Sep 27, 2024
| /**! | ||
| * License header | ||
| */ | ||
Member
Author
There was a problem hiding this comment.
Didn't add html {} before these imports because that would be an invalid input.
RobinMalfait
commented
Sep 27, 2024
| function migrate(root: Root) { | ||
| let baseNode: AtRule | null = null | ||
| let utilitiesNode: AtRule | null = null | ||
| let baseNode = null as AtRule | null |
Member
Author
There was a problem hiding this comment.
I prefer the let baseNode: AtRule | null = null approach, but the types were wrong after the fact (either null or never). Didn't want to change tsconfigs (since they are shared) so solved it this way.
Contributor
There was a problem hiding this comment.
This seems really weird to me because the behavior between the declarations should be identical 🤔
Member
|
@RobinMalfait In this scenario: /**! My license comment */
html {
color: red;
}
@tailwind base;
@tailwind components;
@tailwind utilities;...I think it's important that it compiles to this: /**! My license comment */
@import "tailwindcss";
@layer base {
html {
color: red;
}
}…otherwise any utility classes you add to the |
RobinMalfait
force-pushed
the
fix/improve-import-location
branch
3 times, most recently
from
c8968bb to
6ed62bd
Compare
This test makes sure that we inject the new `@import` in the correct locations.
The idea is that we replace `@tailwind base; @tailwind components; @tailwind utilities;` with the much simple `@import "tailwindcss";`. We did this by adding the `@import` to the top of the file. While this is correct, this means that the diff might not be as clear. For example, if you have a situation where you have a license comment: ```css /**! My license comment */ @tailwind base; @tailwind components; @tailwind utilities; ``` This resulted in: ```css @import "tailwindcss"; /**! My license comment */ ``` While it is not wrong, it feels weird that this behaves like this. In this commit we make sure that it is injected in-place (the first `@tailwind` at-rule we find) and fixup the position if we can't inject it in-place. The above example results in this: ```css /**! My license comment */ @import "tailwindcss"; ``` However, there are scenario's where you can't replace the `@tailwind` directives directly. E.g.: ```css /**! My license comment */ html { color: red; } @tailwind base; @tailwind components; @tailwind utilities; ``` If we replace the `@tailwind` directives in-place, it would look like this: ```css /**! My license comment */ html { color: red; } @import "tailwindcss"; ``` But this is invalid CSS, because you can't have CSS above an `@import` at-rule. There are some exceptions like: - `@charset` - `@import` - `@layer foo, bar;` (just the order, without a body) - comments In this scenario, we inject the import in the nearest place where it is allowed to. In this case: ```css /**! My license comment */ @import "tailwindcss"; html { color: red; } ``` Note that the (license) comment is allowed to exist before the `@import`, therefore we do not put the `@import` above it. This also means that the diff doesn't touch the license header at all, which makes the diffs cleaner and easier to reason about.
1. One of the experimental codemods is new, and not a fix 2. Grouped the codemods together
These don't have to be wrapped in a layer.
RobinMalfait
force-pushed
the
fix/improve-import-location
branch
from
6ed62bd to
67ae8b4
Compare
We already had it.
philipp-spiess
approved these changes
Sep 30, 2024
packages/@tailwindcss-upgrade/src/codemods/migrate-missing-layers.ts
Outdated
Show resolved
Hide resolved
thecrypticace
approved these changes
Sep 30, 2024
…ers.ts Co-authored-by: Philipp Spiess <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR inserts the
@importin a more sensible location when running codemods.The idea is that we replace
@tailwind base; @tailwind components; @tailwind utilities;with the much simple@import "tailwindcss";. We did this by adding the@importto the top of the file.While this is correct, this means that the diff might not be as clear. For example, if you have a situation where you have a license comment:
This resulted in:
While it is not wrong, it feels weird that this behaves like this. In this commit we make sure that it is injected in-place (the first
@tailwindat-rule we find) and fixup the position if we can't inject it in-place.The above example results in this:
However, there are scenario's where you can't replace the
@tailwinddirectives directly. E.g.:If we replace the
@tailwinddirectives in-place, it would look like this:But this is invalid CSS, because you can't have CSS above an
@importat-rule. There are some exceptions like:@charset@import@layer foo, bar;(just the order, without a body)In this scenario, we inject the import in the nearest place where it is allowed to. In this case:
Additionally, we will wrap the existing CSS in an
@layerof the first Tailwind directive we saw. In this case an@layer base. This ensures that utilities still win from the default styles.Also note that the (license) comment is allowed to exist before the
@import, therefore we do not put the@importabove it. This also means that the diff doesn't touch the license header at all, which makes the diffs cleaner and easier to reason about.