Add missing layer(…) to imports above Tailwind directives
#14982
Conversation
RobinMalfait
force-pushed
the
fix/missing-layer-above-tailwind-directives
branch
from
06cebc8 to
3c168fb
Compare
RobinMalfait
force-pushed
the
fix/missing-layer-above-tailwind-directives
branch
2 times, most recently
from
c065675 to
51254b5
Compare
RobinMalfait
force-pushed
the
fix/missing-layer-above-tailwind-directives
branch
from
51254b5 to
1f55220
Compare
At this point it doesn't matter if we are layered or not, but if the stylesheet contains an `@utility`. If it does, then that's a reason to potentially split stylesheets (if necessary).
RobinMalfait
force-pushed
the
fix/missing-layer-above-tailwind-directives
branch
from
1f55220 to
7a41b4e
Compare
| @@ -1571,8 +1698,9 @@ test( | |||
| } | |||
|
|
|||
| --- ./src/components.css --- | |||
| @import './typography.css'; | |||
| @import './typography.css' layer(components); | |||
There was a problem hiding this comment.
This has layer(components) because this is coming from the components file where nothing sits above it (in that file), but there is a @layer components and @tailwind components below it.
if we want to look at as-if it was flattened, then it exists between base and components. This means that if this code existed in the same file we would've generated layer(base) instead. 🤔
There was a problem hiding this comment.
This is also what we get on the Commit template with this PR:
Without knowing the rest of the files, it looks correct. But I wonder if layer(base) is more correct.
There was a problem hiding this comment.
Talked with @philipp-spiess and @thecrypticace and we came to the conclusion that this is the least surprising outcome where you look at CSS files in isolation.
integrations/upgrade/index.test.ts
Outdated
| @import './components.utilities.css'; | ||
| @import './utilities.utilities.css' |
There was a problem hiding this comment.
Maybe we can avoid splitting up this file as tailwind-setup.css is also the Tailwind root. The result here does seem a bit unexpected 🤔
There was a problem hiding this comment.
Good catch, a split made sense, but the split should've stopped at the Tailwind root file indeed. Handled here: b62c499
You can see the difference in the integration tests: 87c0bb2
Re-ran it on the Commit template and the output is the same (because no splits happened there, but it also didn't break anything)
When we detect CSS files that contain `@utility` and non-`@utility` parts, then we split them. We will also split every parent file to ensure the imports are correct. The new files with `@utility` should not result in a `layer(…)` next to the imports. The other CSS might end up with a `layer(…)`
integrations/upgrade/index.test.ts
Outdated
| --- ./src/components.css --- | ||
| @import './typography.css'; | ||
| @import './typography.css' layer(components); | ||
|
|
||
| --- ./src/components.utilities.css --- | ||
| @utility foo { | ||
| color: red; | ||
| } |
There was a problem hiding this comment.
I think that technically since the only non @utility is an @import with a layer(…) that this split shouldn't be required. But that feels like an optimization for a separate PR (if we even need that.).
There was a problem hiding this comment.
I looked into it and it's not a lot of work to make it work because this does feel unexpected right now.
There was a problem hiding this comment.
Ok now I'm happy with this.
We only need to split if the stylesheet contains `@utility` and non-safe CSS. Imports with `layer(…)`, `@layer` blocks and comments are considered safe, everything else is not. The non safe-code might end up in a new import with a `layer(…)` to guarantee that it's injected in the correct spot.
This PR fixes an issue where imports above Tailwind directives didn't get a
layer(…)argument.Given this CSS:
It was migrated to:
But to ensure that the typography styles end up in the correct location, it requires the
layer(…)argument.This PR now migrates the input to:
Test plan:
Added an integration test where an import receives the
layer(…), but an import that eventually contains@utilitydoes not receive thelayer(…)argument. This is necessary otherwise the@utilitywill be nested when we are processing the inlined CSS.Running this on the Commit template, we do have a proper
layer(…)