Repository navigation
Conversation
…icated A later copy of an external @import masks an earlier copy. The duplicate-removal pass replaced the earlier copy in place with a layer list, inside the hoisted import block. That entry printed as a rule in front of or between the @import rules, for example `@media print{@layer a;}`, so every @import after it was invalid. The layer list now remembers the import that it replaced. After the redundant-layer pass, a leading `@layer a;` statement stays as before. A layer list that the next entry makes redundant is dropped. Any other one becomes the import again, with its conditions.
|
Updated 10:07 AM PT - Oct 18th, 2026
❌ @robobun, your commit 3b2f793 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 45675That installs a local version of the PR into your bun-45675 --bun |
|
Status: closed. #47797 supersedes this PR and covers the problem below (checked at 0400ee7). How the problem was reproduced, on bun 1.4.3 and on main: mkdir -p sub
echo '@import "https://example.com/fonts.css" screen;' > sub/a.css
echo '.c0 { color: red }' >> sub/a.css
echo '@import "sub/a.css" layer(theme) print;' > entry.css
echo '@import "sub/a.css" layer(theme) print;' >> entry.css
echo '.c0 { color: blue }' >> entry.css
bun build ./entry.css --minify --outdir out
cat out/entry.css
bun build ./out/entry.css --outdir againThe first build exits 0 and its output starts with |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe CSS import-order optimizer now preserves metadata for conditionally redundant external imports while resolving layer ordering. New tests check emitted CSS across duplicate-import cases, CLI output, shared imports, and CSS reparsing. ChangesCSS External Import Masking
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main change: preserving a valid CSS @import prologue when an external @import is duplicated. |
| Description check | |
The description explains the problem, root cause, fix, verification steps, test coverage, trade-offs, and out-of-scope behavior. It does not use the template headings exactly, but it provides the requ… |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
Full details: Description check
-
Explanation
The description explains the problem, root cause, fix, verification steps, test coverage, trade-offs, and out-of-scope behavior. It does not use the template headings exactly, but it provides the required change summary and verification information in complete detail.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked whether the new unreachable!() arm for Layers::Masked in Layers::to_owned (src/bundler/Chunk.rs) can be hit from the adjacent-layer merge loop: a masked import with no layer condition is always removed by the trailing-trim plus empty-entry check (findImportedFilesInCSSOrder.rs:541-560), and when a layer is present the post-pass at :700 rewrites every surviving Masked entry to Owned, ExternalPath, or drops it before memcpy_and_reset, so no Masked entry reaches the merge loop. The https://example.com strings in the new tests are only unresolved external @ import targets; nothing fetches them.
Extended reasoning...
The change adds a Layers::Masked variant and a post-pass in the CSS import-ordering code so a duplicated external layered @ import is restored instead of printing a layer rule ahead of later imports; it touches no security-sensitive surface. Two inline findings (the second unsafe set_len restore and a pre-existing layer-order fault) are posted separately; this note records the one additional invariant I verified so a human reviewer need not re-derive it.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/bundler/linker_context/findImportedFilesInCSSOrder.rs— pre-existing: a duplicated layered import loses its cascade position whenever a plain external@ importprecedes it, so the bundle orders layers differently from the source. The redundant-layer pass at findImportedFilesInCSSOrder.rs:623 calls a layer entry redundant against any earlier entry with the same (empty) layer-name key, and is_conditional_import_redundant returns true for an emptylaterlist, so@ import "https://x/f.css"; @ import "./t.css" layer(theme); @ import "./b.css" layer(base); @ import "./t.css" layer(theme);prints base before theme. Fix: only treat the layer entry as redundant when the suffix of its conditions beyond the earlier entry's length declares no layer, for both the local and the masked-external case.Why this was flagged
Entry stylesheet:
@ import "https://example.com/fonts.css"; @ import "./theme.css" layer(theme); @ import "./base.css" layer(base); @ import "./theme.css" layer(theme);where theme.css and base.css contain only ordinary rules and no@ layer. The backward pass at findImportedFilesInCSSOrder.rs:407-421 turns the first theme entry into Layers(Borrowed) with conditions [layer(theme)] and an empty name list. In the forward pass the fonts entry gets the empty key at findImportedFilesInCSSOrder.rs:581 and is registered at findImportedFilesInCSSOrder.rs:613; the theme layer entry gets the same empty key at findImportedFilesInCSSOrder.rs:580, and is_conditional_import_redundant(&[layer(theme)], &[]) at findImportedFilesInCSSOrder.rs:623 returns true becauselateris empty (findImportedFilesInCSSOrder.rs:906-910), so findImportedFilesInCSSOrder.rs:679 drops it. Output is@ import "https://example.com/fonts.css";@ layer base{...}@ layer theme{...}: base is declared before theme, while the source declares theme first. The base branch produces the same output.Verification: pre-existing (acknowledged in PR description under "Not in this PR"). At findImportedFilesInCSSOrder.rs:623-626
is_conditional_import_redundant([layer(theme)], [])returnstrue(958) becauselater.len()is 0, andcontinue 'next_forward(679) drops the entry. Nothing in the diff guards thelater.len() == 0case. The base branch already fails identically.
|
Superseded by #47797 |
|
Checked #47797 at 0400ee7 with the inputs of this PR. It covers the problem.
One difference in output size. In 6 of the 18 shapes #47797 keeps a masked copy as an @import "https://example.com/a.css" layer(a) print;
@import "https://example.com/a.css" layer(a) print;main prints the import once. #47797 prints it twice. The output is valid and the cascade is the same. The 6 shapes
|
…plicate imports (#47797) ### What was wrong - `@layer first; @import "i.css" layer(second);` printed `@layer second{...}@layer first;`. A layer gets its priority where its name first appears, so the two layers swapped. The bundler printed the imported files ahead of the whole body of the importer, and the statement is in that body. - The bundler replaces an earlier copy of a file or an external import with a list of its layers, when a later copy covers it. `is_conditional_import_redundant` said "covered" when a condition list was a prefix of the other, and it ignored a `layer()` that only the earlier copy has. So a later bare copy removed rules in another layer, and an unrelated file with no conditions removed the list that declared the layer. - An `@import` that closes a cycle declared no layer. css-cascade-5 says that the layer "is added to the layer order even if the import fails to load the stylesheet". - A list for a duplicate external `@import` could print ahead of the hoisted imports as a block, or between two imports. Each makes later `@import` rules invalid, and a second bundle fails. - `CssImportOrder::hash` leaves out the import conditions. Two entry points with different orders could share one stylesheet. ### The fix - The parser moves the layer names seen before the first `@import` into `StyleSheet.layers_pre_import`. Each visit of a file pushes those names as an entry ahead of its imports, and the file no longer prints those statements. - Each visit of a bare `layer` import gets an id in `Layer.anonymous_id`. `@import "x.css" layer` puts x.css and all its imports into one anonymous layer, and two visits make two layers, also when they are the same rule. `Layer::eql` compares the id, so equality and redundancy checks treat two bare imports as different layers. - Entries with the same anonymous layer print in one `@layer { }` block. Each entry prints only the block edges that are its own: it opens what the next entry uses, or closes what the previous entry opened. The entries stay separate compile results, because each file resolves `url()` from its own import records. The block can sit under other conditions, and these print once too. - A masked copy under a bare `layer` keeps its layer names, because they order the layers of the shared block. - `is_conditional_import_redundant` returns false when the earlier copy has a `layer` condition beyond the later one. No per-chunk mode: this holds for every bundle. - The cycle check pushes an empty layer list under the conditions of the import. - A list prints ahead of the hoisted imports only as a plain `@layer` statement. A duplicate external import with a layer stays an `@import` unless a leading statement or an earlier equal copy declares the layer. - A file's layer names can be inside `@media`, so a file entry no longer removes a list of names. Only a list of the same file is dropped when that file follows it directly. - `compute_chunks` compares the import orders on a hash hit and takes the next free key if they differ. ### Downsides - A file imported both under `layer(x)` and bare now prints twice. Its rules are in two layers, so both copies are needed. An external import in that shape stays two `@import` rules. - Bundles that have a `@layer` statement ahead of an `@import` change bytes. That is the fix. - A file imported twice with a bare `layer` now prints twice, because the copies are in two layers. `css/CSSAtImportConditionsAtLayerBundle` case5 has one more copy in its snapshot, and differs from esbuild's snapshot there. - With `minifyWhitespace` off, a `/* file */` comment inside a shared block prints at column 0. ### Known wrong cases - An external `@import` under a bare `layer` prints as `@import "x.css" layer`, because external imports are hoisted. That layer is not the one of the files that were imported with it, so the single anonymous layer is split in two. The random graph check has no external imports. - A layer that is declared only inside `@media` still gets an unconditional list for a duplicate copy, as on main. ### Tests On a debug build of this branch, `test/bundler/esbuild/css.test.ts`, `test/bundler/bundler_html.test.ts`, `test/regression/issue/20546.test.ts`, `test/regression/issue/28914.test.ts`, `test/bundler/css/` and `test/bake/dev/css.test.ts` give 369 pass, 1 todo, 0 fail. The todo is not about layers. Run on Bun 1.4.3, the first three files give 51 failures, including the new cases. Some of those 41 come from other changes since 1.4.3. `css/CSSAtImportConditionsBundleLOL` now expects one more external `@import`. The old output dropped it only because a duplicate list was removed by an unconditional copy in another layer. A smaller input without that copy prints an `@media` block between two `@import` rules on 1.4.3, and the rule after it is invalid. The random graph check from #41572 (not in this PR), 200 graphs each, seeds from 1: | Options | Right here | Wrong with 1.4.3, right here | Right rule with 1.4.3, another here | | --- | --- | --- | --- | | default | 200 | 147 | 0 | | cycles | 200 | 156 | 0 | | bare `layer` | 200 | 122 | 0 | | empty blocks | 200 | 174 | 0 | | bare `layer`, cycles and empty blocks | 200 | 161 | 0 | The baseline was Bun 1.4.3, not main. The check has no `@media`, `@supports` or `!important`. Fixes #20546 Supersedes #41572 Supersedes #45675 Supersedes #46357
Problem
layer()can makebun buildprint a rule before the hoisted imports:@media print{@layer a;}@import "https://example.com/b.css" layer(b);. Every@importafter it is invalid. Chrome loads none, and Bun's parser rejects the output:@import rules must come before any other rules except @charset and @layer.src/bundler/linker_context/findImportedFilesInCSSOrder.rs:438) overwrites the earlier copy with a layer list, after the hoist. esbuild 0.28.1 does the same. Found by fuzzing, no user report.Fix
Layers::Masked). After the redundant-layer pass, a leading@layer a;stays and a list that the next entry makes redundant is dropped. Any other list becomes the@importagain.test/bundler/esbuild/css.test.ts(19 new cases, 15 fail on main). Alsotest/bake/dev/css.test.ts,bundler_html.test.ts.todocases here.Background
@importcan be that place.@importinto one list and hoists the external imports to the top.data:import (a CSP can block it), always the duplicate (10 identical imports stay 10).Downsides
@import(150 to 178 bytes, example in Notes).CssImportOrdergrows from 104 to 112 bytes, release.textby 2,048 bytes.Notes
Example (one JS entry that imports two stylesheets):
@media print{@layer a;}@import "https://example.com/b.css" layer(b);@import "https://example.com/a.css" layer(a) print;.one{color:red}.two{color:red}@import "https://example.com/a.css" layer(a) print;@import "https://example.com/b.css" layer(b);@import "https://example.com/a.css" layer(a) print;.one{color:red}.two{color:red}Other shapes that printed an invalid prologue (each is a test case):
@import A layer(a) print; @import A layer(a);printed@media print{@layer a;}first. It now prints the one import.supports()in place of the media query printed an@supportsblock first.@layer outer{@layer inner;}first. A layer block is not a layer statement.@import F layer(first); @import A layer(a); @import B layer(b); @import A layer(a);printed@layer a;between two imports. A layer statement is only valid ahead of the first import.Measurements. Release builds of the merge base and of this PR, same commit. Fixtures are 11 small import graphs, 7 of them invalid on main.
data:import as a file (3) or prints a pre-import@layerstatement after the imports (2, see css: emit @layer statements that precede @import before the imported files #41572).CSSImportRuleand Chrome makes 1 request fora.css.find_imported_files_in_css_order: 9,927 to 11,739 bytes.Not in this PR
@import "first.css"; @import A layer(a); @import B layer(b); @import A layer(a);printsbbeforea. Local files have the same fault. This PR does not change those bytes.!importantdeclarations in different layers. That rule is older than this PR and also applies to local files.data:import, as every external import with nested conditions does on main.Maskedarm can hold the condition slice, with nounsafe.