bundler: report a split dynamic import as its input file in the metafile - #38465
bundler: report a split dynamic import as its input file in the metafile#38465robobun wants to merge 6 commits into
Conversation
With --splitting, compute_cross_chunk_dependencies rewrites a dynamic import of another entry point so the printer emits an import of that entry point's chunk: the record's path becomes the chunk's unique key and its source index is cleared. The metafile is generated afterwards, so the "inputs" section listed the import as the chunk's output path with "external": true, and import attributes were dropped. Map each entry point chunk's unique key back to its entry point source and resolve such records like any other bundled import, so the inputs graph links the importer to the imported file (the "inputs" key), as it does without --splitting and as esbuild does. The outputs section already lists the chunk. The break_output_into_pieces pass at the end of generate() only existed to turn the leaked unique key into a chunk path; nothing in the metafile contains unique keys now, so it is removed.
|
Warning Review limit reached
Next review available in: 36 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 (2)
Comment |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit acb6827 has some failures in 🧪 To try this PR locally: bunx bun-pr 38465That installs a local version of the PR into your bun-38465 --bun |
|
Reproduced on bun 1.4.0 and main with the repro in the PR description: with Fix is in this PR (#38465). Tests: test/bundler/metafile.test.ts, three cases that fail on the released build and pass with the change. Current revision (fe75d04, 51d8753 is an empty retrigger): the change is confined to the inputs loop in MetafileBuilder.rs; the CI: on the Buildkite build for 51d8753 every lane that has finished is green (177 jobs; the remaining two are the macOS 14 arm64 test lanes, queued for agents), and the only annotations are tests unrelated to the bundler that failed in a parallel batch and passed on retry. The red |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I checked whether removing the break_output_into_pieces post-pass could leave a unique-key placeholder in the metafile: record.path.text is set to a chunk unique key only at computeCrossChunkDependencies.rs:177 (the case the new map resolves), and generate_chunk_json writes final_rel_path, so no other unique keys reach the joiner.
Extended reasoning...
Verified the invariant the removed post-pass relied on: grep across src/bundler shows the single assignment of a chunk unique key into an import record's path.text is the dynamic-import redirect at computeCrossChunkDependencies.rs:177, which the new entry_point_by_chunk_key lookup covers. The per-chunk metafile_chunk_json fragments (the other borrowed content pushed into the joiner) use final_rel_path for output/cross-chunk/CSS paths, not unique keys. chunk.unique_key is &'static [u8] so put_static_key is the right helper and the map's borrowed keys outlive it. Deferring on approval because dropping a post-processing pass in the linker is the kind of invariant claim a bundler maintainer should confirm.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this and found no issues — the earlier stdout-drain nit and the comment-cop flags have all been addressed. Because the change removes the break_output_into_pieces post-processing pass from the metafile path on the claim that redirected dynamic-import records were its only source of unique keys, a maintainer familiar with the linker should confirm that reasoning before merge.
What was reviewed
- Traced
compute_cross_chunk_dependencies(line 177) — the only site that writes a chunkunique_keyintorecord.path.text, and it always targets an entry-point chunk, so the new map covers every case. - Checked that
generate_chunk_jsonemits onlyfinal_rel_path/path.pretty, never unique keys, so the removed pass had nothing else to rewrite. - Confirmed
chunk.unique_keyis&'static [u8]soput_static_keyis sound; externals and runtime records still fall through unchanged.
Extended reasoning...
Overview
Fixes metafile inputs reporting under --splitting: dynamic imports of bundled files were emitted as the chunk output path with "external": true instead of the source file's input key. The fix builds a unique_key → entry_point.source_index map in metafile_builder::generate and resolves redirected import records through it, then removes the now-dead break_output_into_pieces + intermediate.code() post-pass (including its unsafe { j.detach_lifetime() }). generate drops to &LinkerContext / &[Chunk]; both callers in bundle_v2.rs updated. Three tests added/rewritten in test/bundler/metafile.test.ts.
Security risks
None. Build-time metafile JSON generation; no user-controlled input reaches new code paths beyond what already flows through the bundler.
Level of scrutiny
Moderate-to-high. The bundler linker is load-bearing, and the PR removes a post-processing pass on the argument that it only ever had one job (rewriting leaked chunk unique keys) which is now handled earlier. I traced this: computeCrossChunkDependencies.rs:177 is the only place a chunk's unique_key is written into an import record's path.text, and it uses entry_point_chunk_indices[...] so the target is always an entry-point chunk — exactly what the new map indexes. generate_chunk_json (which produces the outputs fragments joined into the same buffer) only writes final_rel_path and path.pretty, never unique keys. So the removed pass is indeed dead here. But this is the kind of non-local invariant a linker maintainer should sign off on.
Other factors
- The rewritten "metafile tracks dynamic-import imports with code splitting" test previously asserted the buggy behavior (
toMatch(/^\.\/chunk-[a-z0-9]+\.js$/)). Rewriting it is correct — it was certifying a bug — but worth a maintainer's eye since it changes what the test guards. - The new "inputs are the same with and without --splitting" test is strong: it pins the exact
importsarray (including thewith: { type: "json" }attribute), asserts structural equality between the two builds, and separately verifies the entry'soutputs[].importsstill lists the split chunks — so the fix is shown not to regress the outputs graph. - All prior review feedback is resolved: both new spawns now drain stdout in the same
Promise.all, and the long explanatory comment on the map was reduced to a one-line pointer at the lookup site. put_static_keyrequires&'static [u8];Chunk::unique_keyis declared&'static [u8](Chunk.rs:49), so no lifetime concern.
A dynamic import is redirected at entry_point_chunk_index of its target, which is still 0 for an entry point that never received a chunk of its own (an import()ed stylesheet deduplicated by --css-chunking), so such a record can point at a chunk that is not an entry point chunk. Without the break_output_into_pieces pass the metafile printed the raw unique key for it; with the pass it keeps printing the chunk's output path, as before this change. Also pin the import()ed stylesheet edge in the Bun.build test.
There was a problem hiding this comment.
LGTM — the earlier stdout-drain nit and the comment-cop notes are addressed, and the fallback pass is back in fe75d04.
What was reviewed:
- Traced the redirect in
computeCrossChunkDependencies.rs:177(setsrecord.path.text = chunks[i].unique_key, clearssource_index) against the new map — the chunk'sentry_point.source_index()is exactly the original import target, so the recoveredinputsedge is correct. - Checked that externals and runtime imports keep falling through (unique keys carry a random prefix;
Index::RUNTIMEis still special-cased in the path lookup), and thatchunk.unique_key: &'static [u8]satisfiesput_static_key's lifetime bound. - Verified
tempDirusesmkdtempSync, so the two concurrentbuildMetafilecalls with the same prefix don't collide; both new spawns now drain stdout in the samePromise.all.
Extended reasoning...
Overview
Fixes the metafile's inputs graph under --splitting: dynamic imports that were redirected to a chunk's unique key (by compute_cross_chunk_dependencies) are now mapped back to the imported file's source index via a small unique_key → entry_point.source_index() map, so path is the target's inputs key, external is dropped, and with is derived from the target's loader — matching the non-splitting output and esbuild. outputs[].imports (which should point at the chunk) is untouched. Three tests in test/bundler/metafile.test.ts cover the Bun.build API, the CLI (asserting inputs is byte-identical with/without --splitting), and --metafile-md.
Security risks
None. This is JSON report generation over the bundler's own graph; no untrusted input reaches the new map, and unique keys carry a random per-build prefix so a user path can't collide with a map key.
Level of scrutiny
Medium. It's bundler linker code, but the change is confined to how the metafile reports an already-linked graph — it doesn't touch code generation, chunk layout, or any emitted bundle. A wrong answer here degrades a diagnostic report, not runtime behavior. I traced the redirect at computeCrossChunkDependencies.rs:177 and confirmed the map's value (chunk.entry_point.source_index()) is the original import target per the entry_point_chunk_indices assignment in computeChunks.rs. The refactored bounds/RUNTIME checks and the loaders.get(idx) rewrite are behavior-preserving.
Other factors
- All three prior review threads (my stdout-drain nit and two comment-cop hits) are resolved in 2987707 / 1af0c70 / acb6827; fe75d04 restores the
break_output_into_piecesfallback so the--css-chunkingedge case keeps today's output (the CI failure flagged on acb6827 predates that restoration). chunk.unique_keyis declared&'static [u8](arena-erased) soStringHashMap::put_static_keyis the right insert; the map is dropped beforegeneratereturns.- The rewritten
metafile tracks dynamic-import importstest previously asserted the buggy behavior (chunk path + regex match); its replacement asserts the corrected edge and additionally checksoutputsstill points at the chunk, so coverage is strictly stronger. tempDirwrapsmkdtempSync, so the two concurrentbuildMetafile("--splitting")/buildMetafile()calls get distinct directories.
There was a problem hiding this comment.
LGTM — targeted metafile fix, prior review feedback (stdout draining, comment length) addressed.
What was reviewed:
- Traced the mechanism to
computeCrossChunkDependencies.rs:177(clearssource_index, setspath.textto the chunk's unique key) — the map correctly reverses that to the chunk's entry-point source. - Checked
put_static_keycontract:chunk.unique_keyis&'static [u8](Chunk.rs:49), so the borrowed-key store is sound; map is local togenerate. - Confirmed externals still fall through (no matching key, random-prefixed) and the retained
break_output_into_piecespass covers the css-chunking edge case where the redirect targets a non-entry chunk. - Rewritten test previously asserted the buggy chunk-path behavior; updating it in the same PR is correct.
Extended reasoning...
Overview
The PR fixes metafile inputs reporting for dynamic imports under --splitting. compute_cross_chunk_dependencies rewrites each split dynamic import so the printer emits import("<chunk-unique-key>"): it sets record.path.text to the target chunk's unique key and clears record.source_index. The metafile builder ran after that rewrite and, with no source index, fell through to path.text (the placeholder, later resolved to the chunk output path) and marked the edge external: true. The fix builds a small unique_key → entry_point.source_index map over entry-point chunks and consults it when source_index is invalid, then uses the resolved index for path/external/with exactly as bundled imports already do.
Files touched: src/bundler/linker_context/MetafileBuilder.rs (~30 lines net, confined to the generate inputs loop) and test/bundler/metafile.test.ts (one test rewritten to assert the corrected behavior, two added).
Security risks
None. This is a build-time diagnostic report (JSON/markdown). No untrusted input parsing, no auth/crypto/permissions, no runtime code path.
Level of scrutiny
Low-to-medium. The change is confined to metafile output — worst-case regression is a wrong path string in meta.json, not incorrect bundled code. I verified: chunk.unique_key: &'static [u8] satisfies put_static_key's lifetime contract; unique keys are per-chunk-index so no map collisions; genuine externals can't collide with the random-prefixed keys; the break_output_into_pieces fallback is retained so any unmapped placeholder (the css-chunking case in the description) still resolves to an output path rather than leaking. The refactor of the external and with conditions to use target_source_index is behavior-preserving for the pre-existing branches.
Other factors
- All three prior review threads (undrained stdout, two comment-cop long-comment flags) are resolved in commits 2987707, 1af0c70, and acb6827; the current diff reflects those fixes.
- The pre-existing
metafile tracks dynamic-import importstest asserted the buggy chunk-path output; updating it alongside the fix is what REVIEW.md requires. - Tests cover Bun.build API, CLI (
--metafile), and--metafile-md; the with/without-splitting equality assertion is a strong invariant that guards against future divergence. - No CODEOWNERS on bundler paths.
- The robobun CI status references acb6827 (before fe75d04 restored the fallback pass and 51d8753 retriggered); merge remains gated on the latest build going green, but the code as reviewed is correct.
|
#38727 approached the same fall-through from the other side (write the chunk's
The tail is worth deleting rather than keeping as a fallback: |
1 participant
Problem
--splitting, the metafile'sinputssection reports a dynamic import of a bundled file as the chunk's output path and marks it external:--splitting(and in esbuild, with or without splitting) the same import is{ "path": "data.json", "kind": "dynamic-import", "original": "./data.json", "with": { "type": "json" } }.inputsedges (esbuild's analyzer,--metafile-md) lose every dynamic-import edge under splitting: the markdown report counts them as "External imports" and lists the imported module as "(entry point or orphan)".compute_cross_chunk_dependencies(src/bundler/linker_context/computeCrossChunkDependencies.rs:177) rewrites every dynamic import of another entry point so the printer emits an import of that entry point's chunk:record.path.textbecomes the chunk's unique key andrecord.source_indexis cleared.metafile_builder::generateruns after linking and reads the same records, so the import fell through topath.text(the unique key, which thebreak_output_into_piecespass at the end ofgeneratethen turned into the chunk's output path), the cleared source index produced"external": true, and thewithlookup was skipped. bundler: make metafile import paths deterministic and match input keys #34534 fixed the static-import half of this (emit theinputskey for bundled imports) and left this case as the fall-through.Fix
generatebuilds a map from each entry point chunk's unique key to the chunk's entry point source index. A record with no source index whose path is one of those keys resolves to that source and is then emitted exactly like any other bundled import:pathis the target's pretty path (itsinputskey), noexternal,withderived from the target's loader.entry_point_chunk_index[target], and computeChunks.rs:505-518 sets that to the chunk whoseentry_point.source_index()is the target, so the chunk's entry point is the file that was imported. This holds for JS, JSON, and asset targets, and for animport()ed stylesheet (whose entry chunk is a CSS chunk today and gains a JS chunk with the same entry point under bundler: emit a JS chunk for a dynamically imported stylesheet #38319 / bundler: emit the JS chunk for an import()ed stylesheet that is also an entry point #38455, so the edge is unchanged when those land). Which output file serves the import stays inoutputs[].imports, which this change does not touch.break_output_into_piecespass at the end ofgenerateis kept.entry_point_chunk_indexis 0 for an entry point that never received a chunk of its own (today: animport()ed stylesheet deduplicated by--css-chunking), so the linker redirects such an import at chunk 0, which may not be an entry chunk; the map has no entry for it and the pass keeps reporting what it reports today (chunk 0's output path, external) instead of leaking the raw placeholder. Fixing the redirect itself is the business of bundler: emit a JS chunk for a dynamically imported stylesheet #38319 / bundler: emit the JS chunk for an import()ed stylesheet that is also an entry point #38455 (own chunk for animport()ed stylesheet) and bundler: list CSS entry points in the metafile and report copied asset sizes #38341 (CSS entries in the metafile); this PR touches none of the code they change and does not depend on their order."external": true; records pointing at the runtime still fall through topath.text.metafile tracks dynamic-import imports with code splitting(Bun.build API): animport()of a JS file and of a stylesheet both resolve to the imported file's key; previously asserted the chunk path. Also checksoutputsstill points at the JS chunk.metafile inputs are the same with and without --splitting(CLI): a JSON target (with), a second user entry point that is alsoimport()ed, and a file only reachable throughimport();inputsis identical between the two builds, every edge points at an input, and the entry's output lists the three chunks.markdown links dynamically imported modules to their importers with --splitting(--metafile-md).--splitting --css-chunkingwith animport()ed stylesheet produces the same metafile as 1.4.0.Background
inputsmaps each source file to the imports found in it (edges between source files, plus externals);outputsmaps each emitted file to the files it contains and the other outputs it imports. Bun emitsoutputsper chunk during chunk generation and assemblesinputsafterwards inmetafile_builder::generatefrom the import records left in the graph.src/ast/import_record.rs).source_indexidentifies the bundled file it resolved to; when it is invalid the printer printspath.textverbatim as an external import. That is why the splitting redirect both clearssource_indexand replacespath.text: it is how the linker asks the printer forimport("<chunk>").{random 64-bit hex}{kind}{index}) each chunk and asset gets before its output path is known; code is printed with the placeholder andbreak_output_into_piecessubstitutes the final path at the end.chunk.unique_keyis a chunk's placeholder, and an entry point's chunk records that entry point inchunk.entry_point.Repro
Before (bun 1.4.0 and main),
inputs["entry.ts"].imports:[{ "path": "./data-jn2krqnp.js", "kind": "dynamic-import", "original": "./data.json", "external": true }]After:
[{ "path": "data.json", "kind": "dynamic-import", "original": "./data.json", "with": { "type": "json" } }]outputs["./entry.js"].importsstill contains{ "path": "./data-jn2krqnp.js", "kind": "dynamic-import" }in both cases.Earlier revision
The first revision of this PR also deleted the
break_output_into_piecespass at the end ofgenerate, on the grounds that the inputs loop now resolves every redirected record. Review turned up the--css-chunkingcase above, where the redirect points at a chunk that is not an entry chunk and the deleted pass was what kept the raw placeholder (...C00000000) out of the metafile, so the pass is back andgenerate's signature and its callers in bundle_v2.rs are unchanged from main.