runtime: make CSS default export {} to match bun build - #35163
Conversation
Importing a .css file at runtime fell through to the file-loader fallback in transpile_source_code_inner and exported the absolute path as a string. The bundler emits an empty-object JS stub for the same import (esbuild parity), so bun run and the output of bun build --target=bun disagreed on typeof (string vs object). Give the css loader its own early return in the fallback arm that exports an empty object. The auto-watch registration stays in place so --hot continues to pick up .css edits.
|
Reproduced with CI on 786f6da (build 78022) is green for this diff. Remaining red is unrelated:
Ready for review. |
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Updated 2:22 PM PT - Jul 22nd, 2026
❌ @robobun, your commit 786f6da has 3 failures in
🧪 To try this PR locally: bunx bun-pr 35163That installs a local version of the PR into your bun-35163 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the describe.concurrent nit was applied and no further issues found.
What was reviewed:
- Placement of the
L::Csscheck in the_fallback arm: sits after thedisable_transpilyingearly-return, after auto-watch registration (so--hotstill tracks .css edits), and after theglobal_objectnull-check — mirrors the sibling file-loader path exactly. create_empty_object(global, 0)matches the existing empty-JSON precedent at jsc_hooks.rs:2705 and cannot throw; theResolvedSource { .. ExportDefaultObject }shape is identical to the Html and file-loader arms.- Tests: separate
tempDirroots, all pipes drained concurrently, stderr/stdout asserted before exitCode,describe.concurrentapplied.
Extended reasoning...
Overview
Adds a 12-line if matches!(loader, L::Css) branch inside the catch-all arm of transpile_source_code_inner (src/runtime/jsc_hooks.rs) that returns JSValue::create_empty_object as the module's jsvalue_for_export instead of falling through to the file-loader path-string export. Ships a new 52-line parity test at test/js/bun/css/css-loader.test.ts covering both bun run and bun build --target=bun output.
Security risks
None. No untrusted input parsing, no allocation sizing from external data, no new FFI surface. create_empty_object is a non-throwing JSC helper already used identically for empty JSON/TOML results (jsc_hooks.rs:2705).
Level of scrutiny
Low-to-medium. This is a small, mechanical parity fix that copies the exact ResolvedSource construction pattern from the adjacent L::Html arm (lines 3302-3308) and the file-loader tail (lines 3432-3438), differing only in the exported value. The branch is placed after disable_transpilying, after the 'auto_watch block, and after the global_object.is_null() guard, so hot-reload registration and the not-supported error path are preserved unchanged. It is a user-visible behavior change (CSS default export goes from path-string to {}), but it is fixing a divergence where bun build --target=bun already emits {} — so bundled code already sees this shape and this brings bun run into agreement (esbuild parity, per the PR description).
Other factors
The prior review round left one nit (use describe.concurrent), which was applied in 560d400 and is reflected in the current diff. The tests follow repo harness conventions: tempDir with independent roots, bunEnv/bunExe, concurrent pipe draining via Promise.all, and stderr/stdout assertions before exitCode. The PR description shows USE_SYSTEM_BUN=1 fails and bun bd passes. The bug-hunting system found no issues on this revision.
There was a problem hiding this comment.
No issues found — the new L::Css branch mirrors the sibling Html/file-loader ResolvedSource construction exactly, sits after the auto-watch registration and the global_object null-guard, and routes through the existing ExportDefaultObject synthetic-module path. Deferring only because this changes the user-visible value of import x from './foo.css' at runtime (path string → {}); a maintainer should confirm that break is intended even though the docs already say the import "returns no value".
What was reviewed:
- Placement in
transpile_source_code_inner's_arm — auto-watch still fires,disable_transpilyingearly-return still applies,globalderef is guarded. ExportDefaultObjecttag →generateJSValueExportDefaultObjectSourceCodegcProtects the empty object across the provider call, so no GC hazard.import-empty.test.jsupdate matches the new contract;hot.test.ts'shot-file-loader.csscase only does a bareimport(no default binding), so unaffected.docs/runtime/file-types.mdxalready documents CSS imports as returning no value — the old path-string behavior was undocumented fall-through.
Extended reasoning...
Overview
Adds a matches!(loader, L::Css) early-return inside the file-loader catch-all arm of transpile_source_code_inner (src/runtime/jsc_hooks.rs) so that importing a .css file at runtime yields export default {} instead of export default "<abs path>". This matches what bun build --target=bun already emits and what esbuild does. Also adds test/js/bun/css/css-loader.test.ts (two concurrent subprocess tests proving run/build parity) and updates the existing import-empty.test.js CSS assertion from path-string to {}.
Security risks
None. No untrusted input parsing, no new FFI surface. The new branch reuses JSValue::create_empty_object, input_specifier.dupe_ref(), create_if_different, and ResolvedSourceTag::ExportDefaultObject — all identical to the neighboring file-loader/Html arms. The ExportDefaultObject C++ path (generateJSValueExportDefaultObjectSourceCode) already gcProtects the value across the synthetic-provider closure, so the freshly allocated empty object cannot be collected before it's exported.
Level of scrutiny
Medium. The native change is 14 lines and mechanically mirrors sibling arms, so implementation risk is low. However, it lives in the module-loader hot path and changes what an existing import form returns to user code — that's a user-visible behavior change, even if the old behavior (leaking the absolute filesystem path as a string) was undocumented and inconsistent with bun build. docs/runtime/file-types.mdx already states the CSS import "returns no value; it's only used for its side effects", so the change aligns runtime with docs and bundler rather than introducing a new contract. The known .module.css divergence is now honestly documented in the code comment per prior review feedback.
Other factors
Both earlier review nits (make the tests describe.concurrent; scope the comment to plain .css and note the .module.css gap) were applied and the threads are resolved. Tests follow harness conventions (tempDir, bunEnv/bunExe, await using, concurrent pipe drain, stderr/stdout asserted before exitCode) and the PR shows fails-on-system-bun / passes-on-debug-build verification. I checked test/cli/hot/hot-runner.js — it does a bare import "./hot-file-loader.css" with no default binding, so the hot-reload test is unaffected. Deferring rather than approving purely because a change to runtime import semantics — however small and well-justified — is the kind of thing a maintainer should glance at before it ships.
2 participants
Repro
bun runand the artifact produced bybun build --target=bundisagreed on the default export of a plain.cssimport: the runtime returned the absolute file path as a string, the bundler emitsvar s_default = {}.Cause
transpile_source_code_innerinsrc/runtime/jsc_hooks.rshas noLoader::Cssmatch arm, so.cssfalls through to the_catch-all (the file loader) which exportspath.textas a string. The bundler'sParseTaskbuilds an empty-object lazy export for the JS stub of a CSS entry, matching esbuild.Fix
Return an empty object for the css loader in the fallback arm, after the existing auto-watch registration so
--hotkeeps picking up.cssedits.require, dynamicimport(), and Worker imports all go through the same path and now agree with the bundled output.Verification
The existing
should hot reload when hot-file-loader.css is overwrittentest still passes.[review] gate passed · iteration 1 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file