Repository navigation
bake: print CSS chunks with browser targets - #37056
Jarred-Sumner merged 1 commit into
Conversation
WalkthroughChangesThe bundler now selects a separate CSS target. Bake builds use Bake CSS target
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 741-746: Update the CSS test around the existing css assertions to
verify that the fetched output still contains both the `@import` directive and
https://example.com/x.css before checking media-range conversion. Keep the
current min-width and >= assertions unchanged.
🪄 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: c7be8c61-3a0c-4c96-885f-49d06d383cac
📒 Files selected for processing (7)
src/bundler/LinkerContext.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
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate, though the two PRs are deliberately adjacent. #37051 fixes the minify-time half of this divergence: vendor prefixing and selector downleveling are assigned during The |
There was a problem hiding this comment.
No bugs found. This is a focused, well-tested fix, but it introduces a new is_bake_build() predicate and a css_target split from target — a small design decision (bake CSS always prints for Target::Browser) that a maintainer should sign off on, especially given the stated interaction with #37051's minify-time half.
What was reviewed:
- Verified
css_target()returnsself.targetfor non-bake builds, so plainbun build --target=bun|nodeCSS output is unchanged. - Confirmed all bake transpiler setup paths (
DevServer.rs,bake/mod.rs,bake_body.rs) setoptions.framework, so the predicate fires for both dev server and--appproduction. - Checked the four
for_bundler_targetprint sites are the complete set in the linker; the two remaining call sites (transpiler.rs,ParseTask.rs) are parse/minify-time and intentionally deferred to #37051. - The external-
@importtest'shttps://example.comURL is preserved as text in the output CSS and never fetched.
Extended reasoning...
Overview
Adds a css_target: Target field to LinkerOptions, populated from a new BundleOptions::css_target() helper that returns Target::Browser when is_bake_build() (framework set or dev server present) and self.target otherwise. The four CSS print sites in generateCompileResultForCssChunk.rs and prepareCssAstsForChunk.rs switch from c.options.target to c.options.css_target. Three new tests cover the dev-server-served CSS chunk, an external @import with a media range condition, and bun build --app output.
Security risks
None. No untrusted-input parsing, no auth/crypto/permission surface. The change only alters which browser feature-set the CSS printer downlevels for.
Level of scrutiny
Medium. It's a bundler output-behavior change, but scoped narrowly: non-bake builds are provably unaffected (css_target() falls through to self.target), and within bake builds the effect is strictly more downleveling — which is the fix. Targets::for_bundler_target already maps Target::Browser → browser_default() and Target::Bun/Node → runtime_default(), so the mechanism is well-understood.
Other factors
- The
is_bake_build()predicate is new reusable surface. It's simple and its two inputs (framework,dev_server) are only set from bake code paths (verified by grep), but a maintainer should confirm this is the right long-term shape versus, say, threading an explicit CSS target from the bake caller. - The PR description positions this as the print-time complement to #37051's minify-time fix; a reviewer with context on that PR should confirm the two compose as described.
- Tests look solid: they assert both the positive (
.a .b,min-width) and negative (light-dark(,>=) invariants, and per the description fail on the current release.
f1aae90 to
0d84808
Compare
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.
0d84808 to
25c6314
Compare
|
Restructured after a deeper review pass found that this change is not independent of #37051 after all: the light-dark() polyfill injects its variable definitions at minify time and rewrites references at print time. Fixing only the print targets produced a half-polyfill for server-imported stylesheets that declare color-scheme (references with no definitions, which computes to an invalid value and drops the declaration in every browser), worse than the raw light-dark() it replaced. This PR is now based on the branch for #37051, so minify and print resolve CSS targets through the same css_target() helper and the diff here is only the print-side plumbing plus tests. Merge #37051 first. Also added tests for the two print sites the original three tests did not reach (the data: URL wrapper for nested import conditions and the bare layer-statement path), and a polyfill test with color-scheme declared that fails on either PR alone and passes with both. The PR description has the details. |
There was a problem hiding this comment.
LGTM — small, mechanical print-target plumbing with per-site test coverage.
What was reviewed
- The four linker CSS print sites (three
CssImportOrderKindarms + the nested-conditiondata:wrapper) now readc.options.css_target; verified no otherfor_bundler_target(c.options.target)call remains in the linker. LinkerOptionshas oneDefaultimpl and one population site (bundle_v2.rs) — both updated; non-bake builds are unaffected sincecss_target()returnsself.target.- The
transpiler.rs:3070print site was checked and left alone deliberately — it's the single-fileTranspilertransform path (minifies with default targets too), not a bake/linker chunk path. - The external-
@importtests referencehttps://example.com/x.cssbut the URL is preserved as an@importin the output and never fetched (hermetic).
Extended reasoning...
Overview
Adds a css_target: Target field to LinkerOptions, populated from BundleOptions::css_target() (introduced in the base PR #37051) at the existing option-copy site in bundle_v2.rs. The four CSS print sites in the linker (generateCompileResultForCssChunk.rs × 3 arms, prepareCssAstsForChunk.rs × 1) switch from c.options.target to c.options.css_target. The css_target() doc comment is extended to note the minify/print agreement invariant. Six new tests exercise each print site plus the cross-phase light-dark() polyfill, in both dev-server and bun build --app modes.
Security risks
None. No parsing of untrusted input, no auth/crypto/permission surface. The change threads an existing enum value through an existing options struct.
Level of scrutiny
Low-to-medium. The native diff is one struct field (Copy enum, defaulted to Target::Browser matching the neighboring target default), one assignment, and four one-token substitutions. The behavioral change is scoped to bake builds only: for every non-bake bundle css_target() returns self.target, so css_target == target and the print sites see the same value they did before. I grepped for all for_bundler_target and LinkerOptions construction/population sites to confirm nothing was missed; the only remaining .options.target-fed CSS printer is in transpiler.rs on the single-file transform path, which is not a bake code path and pairs with MinifyOptions::default() anyway.
Other factors
- Test coverage is unusually thorough: one test per print-site arm (SourceIndex, ExternalPath, Layers, and the
prepareCssAstsForChunkdata:-URL wrapper), plus a polyfill test that pins the minify+print agreement, plus a production-build variant. Each asserts the specific downleveled output shape. - The external-URL tests do not violate hermeticity — the URL is emitted as a preserved
@importstring, not fetched. - All prior bot comments (CodeRabbit assertion strengthening, comment-cop) are resolved; the retained doc comments are field disambiguation / invariant notes, not workaround excuses.
- The PR is stacked on #37051 and states it must not land alone; that's a merge-order constraint the author has documented, not a code defect in this diff.
|
CI on 25c6314: 195 of 196 jobs passed. The one red job (debian 13 x64-asan) fails only on test/js/node/worker_threads/worker-transfer-terminate-stress.test.ts, which is failing on main as well (JSC Ready for review. Merge order: #37051 first, then this. |
775e3da
into
farm/c2ac6d7c/bake-css-server-import-targets
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 --appskips the CSS downleveling that a plainbun build index.htmlapplies for the default browser targets.Repro (
index.htmllinkingstyles.css):bun build index.htmlemits the downleveled form:while
bun index.html(dev server) serves the stylesheet raw: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 ingenerateCompileResultForCssChunk.rs, plus the nested-conditiondata:URL wrapper inprepareCssAstsForChunk.rs) built those options fromTargets::for_bundler_target(c.options.target), andLinkerOptions.targetis 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::minifyinjects the--buncss-light/--buncss-darkdefinitions and the@media (prefers-color-scheme: dark)toggle rule (gated on the minify targets), and printing rewriteslight-dark(a, b)tovar(--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 withcolor-scheme: light darkwould emit references with no definitions, andvar(--buncss-light, #fff) var(--buncss-dark, #000)substitutes to the invalid#fff #000, dropping the declaration in every browser (worse than the rawlight-dark(), which at least works in browsers that support it). With #37051 underneath, minify and print resolve targets through the sameBundleOptions::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 fromBundleOptions::css_target()(from bake: apply browser CSS targets to stylesheets imported on the server #37051) at the existing option-copy site, used by the four CSS print sites.LinkerOptions.targetis untouched: JS decisions (postProcessJSChunk, HMR runtime selection) still key off it.css_target()and the new field state the minify/print agreement requirement.Plain
bun buildis unchanged: without a framework or dev server,css_target()returns the bundle target, so--target=browserprints as before and--target=bun/--target=nodekeep 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 nolight-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@importcondition 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 adata:URL stylesheet, and the condition inside the wrapper is downleveled (theprepareCssAstsForChunkwrapper 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": withcolor-scheme: light darkdeclared, the served CSS contains the variable definitions, theprefers-color-schemetoggle, and the rewritten references. This is the test that requires bake: apply browser CSS targets to stylesheets imported on the server #37051 and this change together.test/bake/dev/production.test.ts"css is printed with browser targets":bun build --appoutput 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 thetest/bundler/csssuite pass with a debug build of this branch.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