Repository navigation
Conversation
In bake builds (dev server or server components), every stylesheet is emitted into the client bundle and served to the browser, but the CSS minify pass in ParseTask used the importing graph's target. A stylesheet reached through a server-graph import was minified with runtime targets, so vendor prefixing and selector downleveling were missing from the served output, while the same file linked from an HTML route got them.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughBake builds now use browser CSS targeting and adjusted client transpiler switching. Server Components builds share non-code module mappings across graph maps. Development and production tests validate CSS transformations, single CSS emission, and duplicate-path handling. ChangesBake CSS pipeline
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bake/dev/css.test.ts`:
- Around line 695-722: Add a separate production or server-components-only test
for the CSS browser-target processing path with the development-server flag
disabled, reusing the scenario and assertions from “css imported on the server
gets browser-target processing.” Keep the existing devTest to cover the
has_dev_server branch, and configure the new variant to exercise the
server_components branch in ParseTask.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 66ef4767-7eb3-4711-8510-6a274523ffc8
📒 Files selected for processing (2)
src/bundler/ParseTask.rstest/bake/dev/css.test.ts
The cross-graph dedupe for non-JavaScript files in server-components builds only ran on the onResolve-plugin fallback path (run_resolver), so a stylesheet reached from two graphs was bundled once per graph. With CSS now minified with browser targets in every graph, those copies are byte-identical and collide on the same content-hashed output path, failing the build with DuplicateOutputPath. Mirror the sharing in process_resolve_queue so one stylesheet gets one source index and one chunk no matter how many graphs import it. Also centralize the bake-build predicate as BundleOptions::is_bake_build (framework presence or dev server, covering production builds of frameworks without server components) and the CSS target choice as BundleOptions::css_target.
…paths run_resolver and process_resolve_queue now call share_non_js_source_index_across_graphs instead of carrying verbatim copies of the registration. The helper also settles the HTML exclusion in one place: server-side HTML imports become per-target manifest modules, so HTML files are never shared across graphs on either path.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/bundler/bundle_v2.rs:6541-6544— The two halves of the fix use mismatched predicates:css_target()fires wheneveris_bake_build()(i.e.framework.is_some()), but this dedup block is still gated onserver_components. In a productionbun build --appof a framework that setsclientEntryPointbut omitsserverComponents, a stylesheet reachable from both the client entry and a server route now minifies identically in both graphs (percss_target()) but still gets two source indices — so it hitsDuplicateOutputPath, whereas before this PR the two copies differed in vendor-prefix bytes and hashed apart. Gating this block (and itsrun_resolvermirror at ~2415) onis_bake_build()matches the predicate the PR already introduced; the body already handlesserver_components == Nonevia.and_then/.is_some_and.Extended reasoning...
What the mismatch is
Commit 8feb1b5 widened the CSS-target predicate from
server_components || has_dev_server()tois_bake_build() = framework.is_some() || has_dev_server(), specifically so production builds of frameworks that don't configureserverComponentsalso get browser CSS targets (options.rs:1573, and theis_bake_build()doc comment says exactly this). But the cross-graph source-index sharing block added here atbundle_v2.rs:6541— the other half of the fix, whose job is to prevent the identical-output-collision that browser-target minification creates — is still gated on the narrowerself.transpiler.options.server_components. So there is now a slice of bake builds where CSS is forced to minify identically across graphs but is not deduped across graphs.The regression scenario
A custom bake framework defines
fileSystemRouterTypes[n].clientEntryPoint(soentry_clientis set) and omitsserverComponents(optional perbake.d.ts:108;Framework::from_jsatbake_body.rs:822leaves itNone).bun build --appis run.production.rs:551-553registersentry_serverasSide::Serverandentry_clientasSide::Client;enqueue_entry_points_bake_productionmaps these toTarget::BunandTarget::Browserrespectively. Two graphs are live.build_graphsisEnumMap<Target, PathToSourceIndexMap>(Graph.rs:60), so each graph has its own path→index map regardless ofserver_components.- The client entry (Browser graph) and a server route (Bun graph) both transitively reach
import "./styles.css"— the standard isomorphic-SSR shape where the hydration entry renders the same component tree as the server. - Each graph's resolve misses
styles.cssin its own map;process_resolve_queueallocates a fresh source index per graph. The new sharing block at line 6541 is skipped becauseoptions.server_components == false(bake_body.rs:1197sets it fromself.server_components.is_some()). - Both parse tasks minify with
topts.css_target().frameworkis set on every bake transpiler (bake_body.rs:1229), sois_bake_build()is true on both the server and client transpilers → both returnTarget::Browser→ byte-identical output. - Two identical CSS chunks → same content-hashed output path →
error.DuplicateOutputPath.
Why this is a regression introduced here
Before this PR, step 4 minified the server-graph copy with
topts.target = Bun(no browser targets → no vendor-prefix bits) and the client-graph copy withBrowser. For any stylesheet with a target-gated feature (:fullscreen,user-select,appearance, etc.) the two chunks differed, hashed to different output paths, and the build succeeded — wastefully emitting two files, but successfully. After this PR both copies are identical and the build fails. (For plain CSS with no target-affected features the collision was already possible pre-PR, so strictly this widens an existing failure window to include essentially any real-world stylesheet.)Why the tests don't catch it
All three new production tests use
framework: "react", which configuresserverComponents(bake_body.rs:527); the dev test usesminimalFramework, which also sets it. No test exercises a framework withclientEntryPointand withoutserverComponents.Fix
Gate the block on the same predicate
css_target()uses:if self.transpiler.options.is_bake_build() && !loader.is_javascript_like() && loader != Loader::Html {
The body already degrades correctly when
server_componentsisNone:separate_ssruses.and_then/.is_some_andand evaluates tofalse, so it shares the index between the Browser and main-target maps and simply skips the SSR map — exactly what's needed. The mirrored block inrun_resolver(~line 2415) has the sameserver_componentsgate and would want the same widening (that copy uses.unwrap()onserver_components, so its body would also need the.and_then/.is_some_andtreatment this block already has — another reason to extract the shared helper the sibling review comment suggests).Why nit rather than blocking
The affected configuration — a custom bake framework with
clientEntryPointbut noserverComponents, whose client entry transitively imports CSS also reached from server pages — is a valid documented shape but is narrow in practice: bake is experimental (production serving still throwsTODO), the only built-in framework (react) configuresserverComponents, and the collision partially predates this PR for stylesheets without prefixable features. It's a one-line consistency fix worth folding in, but not worth holding the primary fix for.
css_target() forces identical CSS minify output across graphs in every bake build, so the sharing gate must cover the same set. A framework with a client entry point but no server components satisfied is_bake_build (identical copies) but not the old server_components gate (no sharing), failing the build with DuplicateOutputPath.
|
Good catch on the predicate mismatch: fixed in ef956a3. The sharing helper now gates on the same is_bake_build() predicate as css_target(), so every build that forces identical CSS minify output also shares the source index. Verified against a framework with a clientEntryPoint and no serverComponents: before the change that build died with DuplicateOutputPath, now it emits one downleveled stylesheet. An end-to-end test for that exact shape is blocked on a pre-existing crash in custom-framework production builds (production.rs panics when writing the runtime file for non-react frameworks), which is tracked separately; the react-based tests cover the helper on the server-components path. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bundler/bundle_v2.rs`:
- Around line 267-275: Replace the expect("oom") calls on both
PathToSourceIndexMap::put results in the graph insertion flow with
unwrap_or_oom(), covering the primary ta insertion and the conditional
separate_ssr tb insertion while preserving the existing arguments and control
flow.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e36ef143-92d2-4f78-af08-c33408e81556
📒 Files selected for processing (1)
src/bundler/bundle_v2.rs
In bake builds (dev server and bun build --app) the primary transpiler targets the server, and the linker's CSS print sites built PrinterOptions from that target. With no browser targets, print-gated CSS transforms (nesting compilation, light-dark() reference rewriting, hex-alpha fallbacks, media range syntax downleveling) never fired, so every bake stylesheet was emitted raw while the same file built via plain 'bun build index.html' was downleveled for the default browser targets. Add LinkerOptions.css_target, populated from BundleOptions::css_target() (browser for bake builds, the bundle target otherwise), and use it at the four CSS print sites. JS decisions keep using LinkerOptions.target. Built on top of the branch for #37051: the light-dark() polyfill injects its variable definitions at minify time and rewrites references at print time, so the minify targets (fixed there) and the print targets (fixed here) must agree or the output is a half-polyfill that drops the declaration in every browser.
In bake builds (dev server and bun build --app) the primary transpiler targets the server, and the linker's CSS print sites built PrinterOptions from that target. With no browser targets, print-gated CSS transforms (nesting compilation, light-dark() reference rewriting, hex-alpha fallbacks, media range syntax downleveling) never fired, so every bake stylesheet was emitted raw while the same file built via plain 'bun build index.html' was downleveled for the default browser targets. Add LinkerOptions.css_target, populated from BundleOptions::css_target() (browser for bake builds, the bundle target otherwise), and use it at the four CSS print sites. JS decisions keep using LinkerOptions.target. Built on top of the branch for #37051: the light-dark() polyfill injects its variable definitions at minify time and rewrites references at print time, so the minify targets (fixed there) and the print targets (fixed here) must agree or the output is a half-polyfill that drops the declaration in every browser.
The plugin success arm created per-graph source indices for files whose path came straight from an onResolve plugin, bypassing the cross-graph sharing the other two resolve paths go through. Call the shared helper there as well so a plugin-resolved stylesheet reached from several graphs gets one chunk.
There was a problem hiding this comment.
No issues found on this revision — all prior threads (predicate scope, helper extraction, third resolve-path sibling, print-time linker sites) have been addressed or explicitly deferred in the PR description. Deferring to a human because this changes existing behavior in the bundler's cross-graph resolve machinery: the run_resolver sharing gate widens from server_components to is_bake_build() and now excludes HTML, which is a semantic change on a pre-existing path rather than a purely additive fix.
What was reviewed:
css_target()/is_bake_build()predicate — verifiedframeworkis set on every bake transpiler so the gate is a strict superset of the oldserver_componentsproxy.- Helper placement at all three resolve sites — each call is inside the
!found_existingbranch after the source index write, and the(ta, tb)arms never alias the just-written map slot. - HTML exclusion in the shared helper — narrows behavior on the
run_resolverpath relative to the pre-existing block; author's rationale (per-target manifest modules) is documented on the helper. - Tests cover dev-server, production server-graph import, and both
'use client'DuplicateOutputPath collision shapes.
Extended reasoning...
Overview
The PR fixes importer-dependent CSS compilation in bake builds: stylesheets imported from server-graph files were minified with runtime targets (no browser downleveling), while the same files linked from HTML got browser targets. It also fixes a related DuplicateOutputPath failure where the same stylesheet reached from multiple graphs (client + SSR) produced per-graph copies.
Files touched: src/bundler/options.rs (adds is_bake_build() and css_target() helpers on BundleOptions), src/bundler/ParseTask.rs (uses css_target() at the CSS minify site and is_bake_build() in place of an inline condition), src/bundler/bundle_v2.rs (extracts share_non_js_source_index_across_graphs from the pre-existing run_resolver block and calls it from three resolve paths), and four new tests in test/bake/dev/{css,production}.test.ts.
Security risks
None. This is compile-time bundler graph bookkeeping and CSS target selection; no user input parsing, no auth, no network.
Level of scrutiny
Medium-high. The css_target() and is_bake_build() additions are straightforward and well-encapsulated. But the resolve-path change is not purely additive: it replaces the pre-existing run_resolver block with a helper whose gate is wider (is_bake_build() vs server_components) and which newly excludes Loader::Html, then applies that same helper to two resolve paths that previously did no cross-graph sharing at all (process_resolve_queue and the onResolve success arm). The bundler's per-target path_to_source_index_map interactions are subtle — a wrong entry there produces mislinked chunks or duplicate parses rather than a compile error — and bake production builds are still evolving (the PR itself notes a pre-existing crash blocking one test shape). A maintainer familiar with the bake graph model should confirm the gate widening and HTML exclusion are correct on the run_resolver path.
Other factors
I've reviewed this PR across four iterations; each finding (predicate not covering framework-without-server-components, duplicated block vs helper, missing third resolve sibling, print-time linker sites) was either fixed in a follow-up commit or explicitly documented as deferred in the PR description. The current revision has no outstanding findings. Test coverage is good: one dev-server test and three production-build tests, each exercising a distinct cell of the fix matrix, and the PR description states the wider bake and bundler CSS suites pass. The expect("oom") calls match the file-wide convention (CodeRabbit withdrew that finding). The two Loader → options::Loader signature edits are pure re-qualification (same nominal type).
|
CI status for the final revision (build 89630): every test failure in the run is an unrelated flake that passed on retry or in isolation (no-orphans, next-pages on Windows, napi GC, node-gyp builds on Windows aarch64). The single hard-failed job is a runner infrastructure failure: the darwin 14 x64 agent could not clone the repository (DNS resolution failed for github.com, three attempts), so no tests ran there. No bake, CSS, or bundler test failed on any lane across the three CI runs of this PR. The diff itself is ready for review. |
|
@robobun fix conflicts |
|
Conflicts resolved: merged main. The only conflict was in src/bundler/options.rs where #37068 removed the dead css_import_behavior method next to the two helpers this PR adds; resolved by keeping the removal. Bake css/production/bundle/html and dev-and-prod suites pass on the merged head. |
There was a problem hiding this comment.
No new issues found; all four prior review threads (bake predicate, linker print-site deferral, helper extraction, onResolve success arm) are addressed in the current revision. Deferring to a human because this widens the cross-graph source-index sharing gate in bundle_v2.rs from server_components to is_bake_build() across all three resolve paths — a bundler-core behavioral change worth a maintainer's eyes.
Checked: css_target() only affects CSS minify (not JS DCE); the run_resolver refactor drops the old .unwrap() chain in favor of .is_some_and so non-server-components bake builds don't panic under the wider gate; the Loader → options::Loader signature edits are the same nominal type; the merge with main only dropped the dead css_import_behavior neighbor.
Extended reasoning...
Overview
The PR fixes importer-dependent CSS output in bake builds: stylesheets reached via the server graph were minified with Target::Bun (no vendor prefixing/downleveling) while the same file via HTML/client got browser targets. Two changes: (1) BundleOptions::css_target() forces Target::Browser for CSS minify in any bake build; (2) a new share_non_js_source_index_across_graphs helper registers non-JS files in every graph's path map so client/server/SSR imports of one stylesheet resolve to one source index (fixing DuplicateOutputPath when identical minify output collides on content-hashed paths). The helper is called from all three resolve paths (run_resolver, process_resolve_queue, onResolve-success). Four new tests cover dev-server and bun build --app shapes.
Security risks
None. No user input parsing, no auth/crypto/permissions, no path traversal surface. The change is internal bundler graph bookkeeping and CSS minify target selection.
Level of scrutiny
High — this is bundler-core (bundle_v2.rs resolve paths, multi-graph source-index maps). The gate widening from server_components to is_bake_build() means the sharing now fires in bake builds without server components (custom frameworks with only fileSystemRouterTypes), which is intentional per the PR description but changes behavior for a shape that isn't end-to-end tested (blocked on a pre-existing custom-framework production crash, per the author). The run_resolver refactor also silently changed .unwrap() → .and_then/.is_some_and, which is required for the wider gate to be sound but is easy to miss.
Other factors
This PR has been through four review iterations from me, each addressed: the bake predicate was corrected to framework.is_some() || has_dev_server(), the duplicated block was extracted into one helper, the onResolve-success sibling was covered, and the linker print-site divergence is documented as an intentional deferral in the PR description. CI is green modulo unrelated flakes and one infra failure. Jarred has engaged (requested conflict resolution) but not yet reviewed the substance. Given the bundler-core scope and the untested custom-framework-without-serverComponents shape, a maintainer sign-off is appropriate rather than auto-approval.
Stacked on #37051 (the diff below is relative to that branch; merge that PR first). The two fix the two halves of the same divergence, and the light-dark() polyfill is only correct with both halves in place, so this one must not land alone. ### Problem Every stylesheet served by the bake dev server or emitted by `bun build --app` skips the CSS downleveling that a plain `bun build index.html` applies for the default browser targets. Repro (`index.html` linking `styles.css`): ```css .a { .b { color: light-dark(white, black); } } ``` `bun build index.html` emits the downleveled form: ```css .a .b { color: var(--buncss-light, #fff) var(--buncss-dark, #000); } ``` while `bun index.html` (dev server) serves the stylesheet raw: ```css .a { & .b { color: light-dark(#fff, #000); } } ``` The default browser targets (chrome87/safari14/firefox78/edge88) support neither CSS nesting nor `light-dark()`, so the served stylesheet is broken in the browsers those targets claim to support. The same applies to media range syntax: a preserved `@import url(...) screen and (width >= 500px)` keeps the range syntax in bake builds but is rewritten to `(min-width: 500px)` in plain browser builds. ### Cause Several CSS transforms are decided at print time against `PrinterOptions.targets`: nesting compilation (`StyleRule::to_css_base`), `light-dark()` reference rewriting and hex-alpha color fallbacks, and media interval syntax downleveling. The linker's four CSS print sites (three arms in `generateCompileResultForCssChunk.rs`, plus the nested-condition `data:` URL wrapper in `prepareCssAstsForChunk.rs`) built those options from `Targets::for_bundler_target(c.options.target)`, and `LinkerOptions.target` is copied from the bundle's primary transpiler. In bake builds the primary transpiler is the server one, whose target maps to runtime targets (`browsers: None`), under which every feature counts as supported and none of the print-gated transforms fire. This is the print-time half of the divergence fixed at minify time by #37051 (vendor prefixing, selector downleveling, and the injected half of the light-dark polyfill), whose description explicitly defers this print-side inconsistency. ### Why this is stacked on #37051 The `light-dark()` polyfill spans both phases: `StyleSheet::minify` injects the `--buncss-light`/`--buncss-dark` definitions and the `@media (prefers-color-scheme: dark)` toggle rule (gated on the minify targets), and printing rewrites `light-dark(a, b)` to `var(--buncss-light, a) var(--buncss-dark, b)` (gated on the print targets). If the phases disagree, the output is a half-polyfill: on this change alone, a server-imported stylesheet with `color-scheme: light dark` would emit references with no definitions, and `var(--buncss-light, #fff) var(--buncss-dark, #000)` substitutes to the invalid `#fff #000`, dropping the declaration in every browser (worse than the raw `light-dark()`, which at least works in browsers that support it). With #37051 underneath, minify and print resolve targets through the same `BundleOptions::css_target()` and the full polyfill is emitted. The new polyfill test pins exactly this: it fails on current releases, on #37051 alone, and on this change alone. ### Fix - `LinkerOptions.css_target`, populated from `BundleOptions::css_target()` (from #37051) at the existing option-copy site, used by the four CSS print sites. `LinkerOptions.target` is untouched: JS decisions (`postProcessJSChunk`, HMR runtime selection) still key off it. - Doc comments on `css_target()` and the new field state the minify/print agreement requirement. Plain `bun build` is unchanged: without a framework or dev server, `css_target()` returns the bundle target, so `--target=browser` prints as before and `--target=bun`/`--target=node` keep runtime targets for both minify and print. ### Verification New tests, one per print site plus the cross-phase polyfill, all failing on the current release and passing with this branch: - `test/bake/dev/css.test.ts` "css chunks are printed with browser targets": dev-server-served CSS has nesting flattened and no `light-dark()` (the per-source print site). - `test/bake/dev/css.test.ts` "external css imports are printed with browser targets": media range syntax in a preserved external `@import` condition is downleveled (the external-path arm). - `test/bake/dev/css.test.ts` "nested external css import conditions are printed with browser targets": an external import reached through a conditional internal import is wrapped in a `data:` URL stylesheet, and the condition inside the wrapper is downleveled (the `prepareCssAstsForChunk` wrapper site). - `test/bake/dev/css.test.ts` "css layer statements are printed with browser targets": a duplicated conditional layer import leaves a bare `@layer foo;` statement whose wrapping media condition is downleveled (the layer-statement arm). - `test/bake/dev/css.test.ts` "light-dark() polyfill spans minify and print for server-imported css": with `color-scheme: light dark` declared, the served CSS contains the variable definitions, the `prefers-color-scheme` toggle, and the rewritten references. This is the test that requires #37051 and this change together. - `test/bake/dev/production.test.ts` "css is printed with browser targets": `bun build --app` output has nesting flattened and the full light-dark polyfill. `test/bake/dev/css.test.ts` (21), `test/bake/dev/production.test.ts` (13), `test/bake/dev/html.test.ts`, `test/bake/dev/bundle.test.ts`, `test/bake/dev/sourcemap.test.ts`, `test/bake/dev-and-prod.test.ts`, and the `test/bundler/css` suite pass with a debug build of this branch. <!-- robobun:evidence:begin --> --- **no test proof** · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bake/dev/production.test.ts <!-- robobun:evidence:end -->
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bundler/bundle_v2.rs (1)
2456-2457: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a production Bake regression test for plugin-resolved CSS.
The current production tests cover file-resolved CSS only. Add an
onResolve/onLoadplugin case that imports the same stylesheet across Bake graphs and asserts one CSS asset with noDuplicateOutputPath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bundler/bundle_v2.rs` around lines 2456 - 2457, Add a production Bake regression test covering plugin-resolved CSS: define an onResolve/onLoad plugin that supplies the same stylesheet to both client and server graphs, then assert the build emits a single CSS asset and no DuplicateOutputPath error. Follow the existing file-resolved CSS production test structure and test helpers.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bake/dev/production.test.ts`:
- Around line 749-753: Update the build invocation in the test around Bun.$ and
capture its stderr alongside exitCode. Assert that stderr is empty before
asserting exitCode is 0, preserving the existing bunEnv and subprocess
configuration.
---
Outside diff comments:
In `@src/bundler/bundle_v2.rs`:
- Around line 2456-2457: Add a production Bake regression test covering
plugin-resolved CSS: define an onResolve/onLoad plugin that supplies the same
stylesheet to both client and server graphs, then assert the build emits a
single CSS asset and no DuplicateOutputPath error. Follow the existing
file-resolved CSS production test structure and test helpers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7cb56cd8-dd97-42ae-b5dc-8ee699981a89
📒 Files selected for processing (8)
src/bundler/LinkerContext.rssrc/bundler/ParseTask.rssrc/bundler/bundle_v2.rssrc/bundler/linker_context/generateCompileResultForCssChunk.rssrc/bundler/linker_context/prepareCssAstsForChunk.rssrc/bundler/options.rstest/bake/dev/css.test.tstest/bake/dev/production.test.ts
The onResolve success arm now has an end-to-end test: a config plugin resolves the same specifier to one stylesheet from a page and a 'use client' component, and the build must emit a single downleveled CSS file instead of failing with DuplicateOutputPath. Also guard the print-targets test's build with a stderr error check (bake builds write a banner and progress to stderr, so emptiness is not assertable).
|
CI status for the current head (build 89723): all bake, CSS, and bundler tests pass on every lane. The remaining failures are a worker_threads stress test that also fails on main (SIGABRT on x64-asan, already reported to main-break triage from an earlier run) and two flakes that passed on retry (bun-install-registry on Windows aarch64, symbols.test in the parallel batch). Nothing in the failure set touches this diff. |
There was a problem hiding this comment.
No bugs found, and every earlier review point has been addressed — but this touches bundler resolve-graph internals (widening cross-graph source-index sharing from server_components to is_bake_build() across three resolve paths) and changes CSS output for every bake build, so it's worth a human sign-off.
What was reviewed:
css_target()/is_bake_build()predicates and the four linker CSS print sites plus the ParseTask minify site — minify and print now agree, and non-bakebun buildis unchanged (css_target()falls through toself.target).share_non_js_source_index_across_graphs: gate matchescss_target()'s; the old.unwrap()chain is gone so the widened gate can't panic on frameworks withoutserver_components; HTML excluded on all three paths;expect("oom")matches file convention.- The remaining
for_bundler_target(self.options.target)intranspiler.rsis the single-file runtime transform, not the bake pipeline — intentionally left. - Tests cover dev + production, all three resolve paths (including the plugin success arm), and the light-dark() minify+print halves.
Extended reasoning...
Overview
The PR fixes importer-dependent CSS compilation in bake builds: stylesheets reached through a server-graph import were minified and printed with the server target (Target::Bun, no browser feature set), so vendor prefixing, selector downleveling, nesting flattening, and the light-dark() polyfill were skipped or half-applied. The fix introduces BundleOptions::css_target() (returns Target::Browser in bake builds, else self.target) and BundleOptions::is_bake_build() (framework.is_some() || has_dev_server()), then routes both the ParseTask minify call and the four linker CSS print sites (three in generateCompileResultForCssChunk.rs, one in prepareCssAstsForChunk.rs) through it via a new LinkerOptions.css_target field.
The second half fixes the DuplicateOutputPath failure that the first half would otherwise create: with identical minify output across graphs, per-graph copies of the same stylesheet collide on the content-hashed output path. The pre-existing cross-graph dedupe in run_resolver is extracted into share_non_js_source_index_across_graphs and called from all three resolve paths (run_resolver plugin fallback, process_resolve_queue normal path, and the onResolve plugin success arm), gated on the same is_bake_build() predicate so the two halves cover the same builds.
Security risks
None. No user-controlled input flows into new parsing, path handling, or allocation logic. The change only adjusts which Target constant is passed to existing CSS minify/print machinery and which per-target hashmap a source index is registered in.
Level of scrutiny
Medium-high. Bake is experimental, but bundle_v2.rs resolve-graph bookkeeping is core bundler infrastructure shared with non-bake builds. The gate widening (server_components → is_bake_build()) and the two new helper call sites change graph-map registration behavior; a mistake here could mis-share source indices across graphs. I traced that (a) the helper's early-return guards (!is_bake_build(), is_javascript_like(), Loader::Html) keep non-bake and JS/HTML behavior unchanged, (b) the widened gate replaces the old .unwrap() chain with .and_then/.is_some_and so a bake build without server_components won't panic, and (c) css_target() returns self.target outside bake so plain bun build --target=bun/node CSS output is untouched.
Other factors
This PR went through several review rounds; my three prior inline findings (predicate coverage, dedup into a helper, and the missing third resolve-path sibling) were each addressed with follow-up commits, and the plugin-resolved CSS production test was added to cover the last one. All CodeRabbit and comment-cop threads are resolved. CI on the final revision was reported clean apart from unrelated infra/flakes. The one remaining for_bundler_target(self.options.target) I found in transpiler.rs is in the runtime single-file CSS transform (not the bake bundler pipeline), so it's correctly out of scope. Given the cross-cutting nature of the resolve-graph change and the build-wide CSS output change, deferring to a maintainer for final sign-off rather than auto-approving.
|
Heads-up for rebasing: #39251 changes the same four CSS print sites this PR touches ( |
Problem
In bake builds, the same CSS file compiles through different pipelines depending on who imports it:
Repro (minimal framework,
routes/index.ts):Fetching the URL in
meta.stylesserves the stylesheet untransformed:while the same file linked from an HTML route serves the downleveled form:
So in dev, a stylesheet imported via a server route renders differently from the same stylesheet linked from HTML, and differently from a production build of the app.
The same importer dependence also duplicates work in production builds: with
separate_ssr_graph(the react framework), a'use client'component is parsed into both the client and SSR graphs, and its stylesheet got one source index per graph.bun build --appthen emitted two copies of the same stylesheet, one downleveled and one not. When a page and a'use client'component imported the same stylesheet, the two server-side copies were byte-identical and the build failed outright witherror.DuplicateOutputPath(the output path is content-hashed).Cause
Two halves:
Vendor prefixing and selector downleveling are decided at minify time in
ParseTask:StyleRule::update_prefixassigns each rule'svendor_prefixbits from the minify targets, and the vendor-prefix fan-out at print time only serializes what minify assigned. The minify call usedtopts.target, the target of the graph that imported the file, so a stylesheet discovered through a server-graph import record was minified withTargets::for_bundler_target(Target::Bun), which has no browser targets. In bake builds that choice is wrong: the stylesheet always joins the client output (the dev server routes every CSS chunk into the client graph and serves it to the browser, with only a server-side stub entry when imported on the server).The cross-graph dedupe for non-JavaScript files ("it is silly to bundle index.css depended on by client+server twice",
run_resolverin bundle_v2.rs) only ran on the onResolve-plugin fallback path. The normalresolve_import_records/process_resolve_queuepath registered files per-graph, so each graph got its own copy of the same stylesheet, producing the duplicate chunks above.Fix
BundleOptions::css_target(): in bake builds, CSS minifies with browser targets regardless of the importing graph. The bake predicate isframework.is_some() || has_dev_server(), which also covers production builds of frameworks that don't configure server components (serverComponentsis optional;frameworkis set on every bake transpiler).BundleOptions::is_bake_build()replaces the two verbatim copies of the condition in ParseTask.rs.All three resolve paths (
run_resolver,process_resolve_queue, and the onResolve-plugin success arm) now share one source index across graphs for non-JavaScript files in bake builds, through a single helper. One stylesheet gets one chunk no matter how many graphs reach it, which fixes the newDuplicateOutputPathfailure mode for'use client'stylesheets and the pre-existing one for stylesheets imported from both a page and a'use client'component. The sharing gate is the sameis_bake_build()predicate ascss_target(): the two halves must cover the same builds, or identical copies collide (verified against a framework with a client entry point and no server components, where the build now emits one stylesheet; an end-to-end test for that shape is blocked on a separate pre-existing crash in custom-framework production builds).The print-time half of the divergence landed here too, via the stacked PR bake: print CSS chunks with browser targets #37056 (merged into this branch):
LinkerOptions.css_targetis populated fromBundleOptions::css_target()and used by the linker's four CSS print sites, so print-gated transforms (nesting compilation, media range syntax,light-dark()and hex-alpha fallbacks) also apply in bake builds. The two halves must agree: thelight-dark()polyfill injects definitions at minify and rewrites references at print, and either half alone emits a broken half-polyfill.LinkerOptions.targetitself is untouched; JS decisions still key off it.Plain
bun build --target=bun/--target=nodebuilds are unchanged: there the single target applies to both minify and print, consistently, as before.Verification
New tests, one per affected cell:
test/bake/dev/css.test.ts: serves a dev-server route whose CSS is imported on the server and asserts the served stylesheet contains the downleveled:-webkit-full-screenselector.test/bake/dev/production.test.ts:'use client'component imports a stylesheet: the build succeeds and emits exactly one CSS file (was: two divergent copies before this PR,DuplicateOutputPathwith the minify fix alone);'use client'component import the same stylesheet: the build succeeds and emits exactly one CSS file (was:DuplicateOutputPatheven before this PR);onResolveplugin from both a page and a'use client'component: one CSS file, noDuplicateOutputPath(was:DuplicateOutputPathon the current release), covering the plugin success arm of the sharing;light-dark()emitted bybun build --appcomes out fully downleveled (the print-side tests from bake: print CSS chunks with browser targets #37056, including three dev-server print-site tests incss.test.ts).All fail on the current release except as noted, and pass with this change.
test/bake/dev/css.test.ts,test/bake/dev/production.test.ts,test/bake/dev/html.test.ts,test/bake/dev/bundle.test.ts,test/bake/dev-and-prod.test.ts, and thetest/bundler/csssuite pass with a debug build.Note: the production tests build and inspect the output rather than serving, because the production server path for framework apps is not implemented yet (
Bun.servethrowsTODO: 'development: false' in serve options with 'app').no test proof · iteration 3 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bake/dev/production.test.ts