bundler: drop import attributes from import() calls that code splitting points at a chunk - #38461
bundler: drop import attributes from import() calls that code splitting points at a chunk#38461robobun wants to merge 4 commits into
Conversation
…ng points at a chunk
With --splitting, a dynamic import of a bundled file is rewritten to import
the chunk built for it. The options object the user wrote on the import()
(for example { with: { type: "json" } }) was printed unchanged after the
chunk path, so the runtime applied that loader to a JavaScript chunk and
failed to load it.
Mark the record when the linker points it at a chunk and have the printer
leave the options out for such records. The record's loader is cleared at
the same point, since it described the original file too.
|
Warning Review limit reached
Next review available in: 31 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 (4)
Comment |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 1f0ac82 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 38461That installs a local version of the PR into your bun-38461 --bun |
|
Status: ready for review. CI for the current head (55d788c, build 96248) has every completed lane green (177 of 179 jobs; the remaining two are darwin 14 aarch64 lanes that have been queued for hours); the only entries in its failure list are tests that passed on retry, none of them bundler-related. Reproduced on 1.4.0 with the two-file repro from the report ( Six new tests in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — surgical fix that drops stale import attributes when code splitting repoints an import() at a JS chunk.
What was reviewed:
- Verified bit 10 in
ImportRecordFlagswas unused (gap betweenWAS_ORIGINALLY_REQUIREat 9 andIS_EXTERNAL_WITHOUT_SIDE_EFFECTSat 11). - Confirmed the printer check sits in the "External import()" branch of
print_require_or_import_expr, the only path a chunk-repointed record reaches. - Checked that genuinely external
import()calls are unaffected (flag is set only at the linker rewrite site); theExternalDynamicImportKeepsAttributestest pins this. - Tests cover json/text/toml, both
withandassertforms, and minified output, each asserting on printed output and running the bundle.
Extended reasoning...
Overview
This PR fixes a bug where bun build --splitting kept the user's import attributes (e.g. { with: { type: "json" } }) on a dynamic import() after rewriting it to point at a JavaScript chunk, causing the runtime to try loading the JS chunk as JSON/text/TOML and fail. The fix touches four files: a new POINTS_TO_CHUNK bitflag in src/ast/import_record.rs (bit 10, previously unused), setting that flag and clearing record.loader at the rewrite site in src/bundler/linker_context/computeCrossChunkDependencies.rs, a two-line guard in src/js_printer/lib.rs to skip printing the options object when the flag is set, and four new itBundled tests in test/bundler/bundler_splitting.test.ts.
Security risks
None. This is bundler output correctness — it changes what JavaScript is emitted for a dynamic import that the bundler already resolved and inlined into a chunk. No untrusted input parsing, no auth/crypto/permissions.
Level of scrutiny
Low-to-medium. The production change is ~10 lines and follows an established pattern (bitflag on ImportRecord, checked in the printer — the file already has several such flags). The fix is placed at the correct layer: the linker owns the rewrite and is the only place that knows the record now names a chunk; the printer can't otherwise distinguish a chunk-repointed import from a genuinely external one. Clearing record.loader alongside is defensive hygiene (nothing reads it for import() records today, but it would be stale). The PR description addresses the side-effect question: the parser only attaches an import record to an import() whose options expression is side-effect-free, so dropping the printed options object cannot change runtime semantics.
Other factors
Test coverage is thorough for the change size: three failing-before tests covering json (via both with and legacy assert), text, toml, and minified output — each asserts the printed import() has no second argument via regex and runs the output end-to-end. A fourth test pins that --external dynamic imports still keep their attributes (passes before and after). The PR description also lists the broader bundler/transpiler suites that were run with the debug build. esbuild's behavior (dropping the attribute in this case) is cited as the reference. No outstanding reviewer comments; only bot noise on the timeline.
…cript A dynamically imported .css or .html entry point is pointed at its own .css/.html output rather than at a JavaScript chunk, and the attributes written on the import() still apply to that file, so leave those records alone. Rename the flag to POINTS_TO_JS_CHUNK to match. Cover the loaders an attribute selects on its own (json via assert on a .notjson file, text on a .md file, toml on an extensionless file, file), options held in a variable, an import() rewritten inside a shared chunk, a conditional import() whose external arm keeps the shared options object, and the stylesheet case keeping its attribute.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused bundler fix with thorough test coverage.
What was reviewed:
- Verified bit 10 was an unused gap in
ImportRecordFlags: u16(betweenWAS_ORIGINALLY_REQUIREat 9 andIS_EXTERNAL_WITHOUT_SIDE_EFFECTSat 11). - Confirmed the printer gate sits in the external
import()branch ofprint_require_or_import_expr, which is exactly where records with invalidatedsource_indexland after the linker rewrite. - Checked the
_chunksBackRef aliasing: readingother_chunk.contentalongside the pre-existingunique_keyread is disjoint from whatwalkmutates (import records, chunk_meta), and the field-list comments were updated to match. - Guard tests pin external imports and CSS-chunk imports keep their attributes; the
is_javascript()gate is what makes the CSS case pass.
Extended reasoning...
Overview
Fixes bun build --splitting emitting stale import attributes on import() calls that the linker repoints at a JavaScript chunk (e.g. import("./data-HASH.js", { with: { type: "json" } }), which fails at runtime). Adds ImportRecordFlags::POINTS_TO_JS_CHUNK; the linker sets it (and clears record.loader) when the target chunk's content is JavaScript, and the printer's external import() branch skips the options object when the flag is present. ~20 lines of production code across three files, plus 8 new itBundled tests.
Security risks
None. This is bundler output shaping — no auth, crypto, path traversal, or untrusted-input parsing is touched. Dropping the options expression cannot drop side effects because (per the PR description, and consistent with the parser design) an import record is only attached to import() when its options expression is side-effect free.
Level of scrutiny
Medium. The bundler is production-critical, but this change is narrow and additive: one new flag bit in a pre-existing gap, one write site (the exact loop that already rewrites path.text and source_index for cross-chunk dynamic imports), one read site (the printer branch that already handled import_options). The is_javascript() gate correctly excludes CSS/HTML chunks, and there is a guard test (DynamicImportToCssChunkKeepsAttribute) that fails without the gate. External dynamic imports never enter the rewrite loop, so their attributes are untouched (also guard-tested).
Other factors
- Test coverage is unusually thorough for a bundler fix: the report's repro, minified variant, all attribute-selected loaders (json via
assert, text, toml, file), options-as-variable,import()inside a shared chunk, a conditional import with one external and one chunk arm, plus two negative guards. Each asserts both the printed output shape and runtime behavior. - The comment-cop bot flagged long comments on earlier commits; all were shortened or removed in be085b3/55d788c4 and the threads are resolved. The remaining two-line doc comment on the flag matches the style of neighboring flags.
- The one CI failure (
test/bake/deinitialization.test.tssegfault on Windows) is unrelated to bundler code splitting. - No CODEOWNERS on the touched paths.
1 participant
Problem
bun build --splittingkeeps the import attributes a user wrote on a dynamic import when it rewrites that import to point at a JavaScript chunk:SyntaxError: JSON parse error: Unrecognized token '/'under bun (any--target),ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLEunder node. Same fortype: "text","toml","file", the legacyassert:form, and options passed as a variable (import("./data.json", options)printsimport("./data-HASH.js", options)). Without the attribute the same program builds toimport("./data-jn2krqnp.js")and works. Reproduces on 1.4.0 and main; esbuild drops the attribute in this situation.computeCrossChunkDependencies.rs(walk, the "Rewrite external dynamic imports" loop) repoints the import record at the chunk by replacingpath.textand invalidatingsource_index, but the options object lives on theE::ImportAST node, not on the record (transpose_importinsrc/js_parser/p.rsstores it there). The printer's externalimport()branch inprint_require_or_import_expr(src/js_printer/lib.rs) then prints that object after whatever path the record holds.Fix
ImportRecordFlags::POINTS_TO_JS_CHUNK(bit 10 was unused). The linker sets it where it repoints the record, when the target chunk's content is JavaScript; the printer's externalimport()branch skips the options object when it is set..cssor.htmlfile is an entry point whose chunk is the.css/.htmloutput itself (computeChunks.rs, theentry_point_chunk_indicesloop), so the rewrite producesimport("./styles-HASH.css", ...). Awith { type: "css" }written on that import still describes what is loaded (in browsers it is what makes importing a stylesheet work), so those records are left exactly as on main. If such an import is later given a JavaScript chunk, the gate strips the attribute there automatically.record.loaderfor the records it flags. It held the loader for the original file (from the attribute, or from resolution), and the record now names a JavaScript chunk. Nothing reads it for animport()record on main today; bundler: keep sqlite imports external when the loader comes from the extension or loader map #38275 adds a printer branch that would, and import statements already printwith { type }from this field, so the record should not carry a stale value.type: "file", as an ES module), so re-applying it to the chunk is never right, and the printer cannot tell a chunk import from a genuinely externalimport()without the flag. The flag is set only on this rewrite, so external dynamic imports keep their options (they still load the file the user named), and non-splitting builds are untouched (an inlinedimport()never reaches this branch). Dropping the object cannot drop side effects: the parser only attaches an import record to animport()whose options expression is side-effect free (e_importinsrc/js_parser/visit/visit_expr.rs).type: "text"andtype: "json"on the same path) is still bundled once and both imports now receive that one chunk; that is bundler: bundle a file once per requested loader instead of once per path #37476's subject. The metafile'sinputsentry for a cross-chunkimport()is also unchanged.test/bundler/bundler_splitting.test.ts. Fail on the released binary, pass with this change:DynamicImportWithAttributeToChunk(target bun, the report's repro),DynamicImportAttributesToChunkMinifiedDynamicImportAttributesToChunkAllLoaders: attributes that select the loader on their own (assertjson on a.notjsonfile, text on a.mdfile, toml on an extensionless file,file)DynamicImportOptionsVariableToChunk: options held in a variableDynamicImportAttributesToChunkFromSharedChunk: theimport()sits in a module shared by two entry points, so the rewrite runs in a shared chunkConditionalDynamicImportExternalAndChunk: one options object shared by both arms ofimport(cond ? "external-data" : "./data.json", ...); printed asimport("external-data", { with: ... }) : import("./data-HASH.js")ExternalDynamicImportKeepsAttributes(--externalimport keeps its options) andDynamicImportToCssChunkKeepsAttribute(import("./styles.css", { with: { type: "css" } })still prints the attribute with the.csspath; fails without the content gate).esbuild/splitting,bundler_compile_splitting,bundler_bun,bundler_html,esbuild/default,bundler_edgecase,transpiler/transpiler.test.js,test/js/bun/import-attributes;cargo fmt --all. Manually: the report's repro under bun and, built with--target node, under node;--compile --splitting(the embeddedimport()loses the attribute too; the standalone loader ignored it, so that case already ran); a file imported both statically and dynamically still yields one module instance.Background
source_indexpoints at the bundled module it resolved to; a record without one is printed back out as an import of itspath. Under--splitting, every dynamically imported file becomes an entry point with its own chunk, andcomputeCrossChunkDependenciesrepoints each cross-chunkimport()record at that chunk (itspath.textbecomes the chunk's placeholder key, replaced with the final file name when chunks are written) so the printer emits it through the externalimport()path. This PR adds one more piece of state to that rewrite.Content::Javascript,Content::CssorContent::Html. A JavaScript entry point gets a JavaScript chunk (plus a secondary CSS chunk for any stylesheets it imports); a.cssor.htmlentry point's primary chunk is the stylesheet or document itself, and that is the chunk a dynamic import of such a file is pointed at.{ with: { type: "json" } }second argument ofimport(). At runtime, bun, node and browsers use it to pick how the target file is loaded (bun accepts its loader names there, such astextandtoml). At build time, the parser uses it to set the loader the bundler parses that file with; the file then becomes an ordinary ES module in the graph.ImportRecord::loader: the loader attached to an import site. The printer uses it to re-emitwith { type }on import statements that stay external for--target bun, so the runtime loads the file the way the build intended.Earlier version of this PR
The first push set the flag (then named
POINTS_TO_CHUNK) on every rewritten record, which also strippedwith { type: "css" }from animport()pointed at a.cssoutput. Review caught that; the second push gates the flag on the target chunk being JavaScript, renames it, and adds the loader, variable, shared chunk, conditional and stylesheet tests.[review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file