Repository navigation
bundler: make metafile import paths deterministic and match input keys - #34534
Conversation
When the same module is imported from multiple files, the resolve dedup
path sets record.source_index without rewriting record.path, so the
metafile emitted the original specifier ("./b/shared.js") for most
importers and the resolved absolute path for whichever importer reached
the resolver first. The winner varied with parse-task completion order,
so the JSON differed between runs and the markdown reverse-dependency
map missed most edges.
Emit sources[record.source_index].path.pretty for bundled imports so the
path is always the same string used as the key in "inputs", matching
esbuild's format. Externals and chunk references keep record.path.text.
|
Reproduced with a 30-importer fan-in; 29/30 import paths held the raw Self-review caught that runtime-helper imports (parser-injected Related: #34531 works around the path mismatch in the markdown generator; this PR removes the mismatch at the JSON source. After the catch-up merge with main (4960cf3), all 44 metafile tests and the 12 esbuild-compat metafile tests still pass locally. On CI the only hard failure is a pre-existing ASAN leak in RSA keygen ( |
WalkthroughMetafile import generation now resolves bundled source indices to pretty input paths, preserves fallback paths, and compares ChangesMetafile import path resolution
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Reproduced with a 30-importer fan-in; 29/30 import paths held the raw Related: #34531 works around this mismatch in the markdown generator; this PR removes the mismatch at the JSON source. |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped fix that makes metafile import paths use the same path.pretty string as the "inputs" keys.
What was reviewed:
- Verified
sources[idx].path.prettyis exactly what the"inputs"key generation uses a few lines up, so lookups now align. - Confirmed dynamic-import chunk refs are unaffected:
computeCrossChunkDependencies.rs:178setssource_index = INVALIDwhen rewriting to aunique_key, so they still fall through torecord.path.textand get resolved bybreakOutputIntoPieces. - Checked
import_pathlifetime — it is copied into an ownedVec<u8>viaformat_json_string_utf8beforepush_owned, so no new borrowed data enters the joiner. - The
"original"comparison now correctly uses the emitted path, so the field appears consistently for every importer (covered by the new test).
Extended reasoning...
Overview
Changes ~15 lines in src/bundler/linker_context/MetafileBuilder.rs (the generate() inputs loop) and adds two tests to test/bundler/metafile.test.ts. For bundled import records, the metafile "path" field now emits sources[record.source_index].path.pretty — the same string used as the top-level "inputs" key — instead of record.path.text, which the resolver dedup path leaves un-rewritten. Externals and chunk references (source_index invalid) keep the old behavior. The "original" field comparison is updated to compare against the actually-emitted path.
Security risks
None. This only affects the shape of the --metafile JSON output (developer tooling metadata), not bundled code, module resolution, or any runtime behavior. No untrusted input parsing, no unsafe blocks, no FFI.
Level of scrutiny
Low-to-medium. The change is mechanical: it reuses the exact same path.pretty value the surrounding code already writes as the "inputs" object key, guarded by the same is_valid() / bounds / non-empty checks used elsewhere in the file. I traced the one interaction that could regress (dynamic imports with --splitting, whose path.text holds a unique_key placeholder resolved later) and confirmed computeCrossChunkDependencies.rs invalidates source_index when rewriting those, so they hit the fallback branch unchanged — the existing "metafile tracks dynamic-import imports" test still exercises this. The import_path slice is consumed synchronously by format_json_string_utf8 into an owned buffer, so the existing detach_lifetime() SAFETY invariant is unaffected.
Other factors
The two new tests are well-constructed: 30 fan-in importers with strong assertions (bad filter must be [], every original must be "./b/shared.js", every entry import must be an inputs key), plus a 5-build determinism check. The PR description states both fail on main and all 43 existing metafile tests + 12 esbuild-compat tests pass. No CODEOWNERS on this path, no prior reviewer comments to address.
Index::RUNTIME has pretty path "runtime", which is an internal name and never appears as an inputs key. Fall through to record.path.text for these so the existing "bun:wrap" output is preserved.
There was a problem hiding this comment.
Beyond the inline nit: I checked that dynamic-import chunk references still emit the resolved chunk path — computeCrossChunkDependencies.rs sets source_index = INVALID when rewriting path.text to the chunk unique_key, so the new pretty-path lookup falls through and breakOutputIntoPieces still resolves it (the existing "metafile tracks dynamic-import imports" test covers this). Also confirmed externals fall through (source_index invalid) and the Index::RUNTIME guard matches the top-level inputs filter.
Note there is an overlapping PR #34531 that works around the same mismatch in the markdown generator — worth deciding which lands / whether that workaround becomes dead code.
Extended reasoning...
The one correctness risk I wanted to rule out was code-split dynamic imports: with splitting: true, an import record targeting another chunk has its path.text rewritten to a unique_key that breakOutputIntoPieces later resolves to ./chunk-xxx.js. If such a record still had a valid source_index, the new lookup would emit the source pretty path instead of the chunk path and break the existing dynamic-import test. computeCrossChunkDependencies.rs:178 sets import_record.source_index = Index::INVALID at the same site it writes the unique_key, so the new block's is_valid() check is false and it falls through to record.path.text as before. Externals likewise have invalid source_index, and the RUNTIME guard mirrors the existing top-level inputs skip. Deferring rather than approving because of the #34531 overlap and the user-visible change to when "original" is emitted (now compared against the pretty path, so it appears on essentially every relative bundled import).
There was a problem hiding this comment.
LGTM — the previous nit on the runtime-helper test was addressed in 93ef252 (importPaths now asserts ["bun:wrap"] exactly).
What was reviewed
- Verified bundled imports now emit
sources[idx].path.pretty, the exact string used for top-level"inputs"keys, with defensive fallbacks (RUNTIME, out-of-range, empty pretty) torecord.path.text. - Confirmed split-chunk dynamic imports are unaffected:
computeCrossChunkDependencies.rs:178setssource_index = Index::INVALIDwhen rewriting to a unique_key, so those records fall through and the existing./chunk-*.jstest still holds. - Checked the
"original"comparison update — externals and chunk refs behave identically to before; deduped bundled imports now correctly emit"original"since path ≠ specifier. - Tests guard against vacuous loops (
midImports.length === 30,toEqual(["bun:wrap"])).
Extended reasoning...
Overview
Two files touched. In src/bundler/linker_context/MetafileBuilder.rs, the metafile "inputs[*].imports[*].path" emitter now looks up sources[record.source_index].path.pretty for bundled imports instead of trusting record.path.text, which the resolver's dedup path leaves un-rewritten. The "original" field comparison is updated to use the same computed path. test/bundler/metafile.test.ts gains three tests: 30-way fan-in path consistency + original preservation, 5-build determinism, and a positive assertion that runtime-helper imports emit bun:wrap.
Security risks
None. This is output formatting for a diagnostic JSON file (the bundler metafile). No user-controlled data reaches new code paths; the new lookup indexes into sources with a bounds check and falls back to the previous value on any miss.
Level of scrutiny
Low-medium. The fix is a ~15-line read-only lookup in a reporting path, mirroring exactly how the top-level "inputs" keys are already computed a few lines above (source.path.pretty). I traced the one non-obvious interaction — dynamic imports rewritten to chunk unique_keys — and confirmed computeCrossChunkDependencies.rs invalidates source_index at the same time it rewrites path.text, so those records take the fallback branch and continue to be resolved by breakOutputIntoPieces. Externals have invalid source_index by construction. Runtime-helper imports are explicitly excluded so they keep emitting bun:wrap rather than the internal source name.
Other factors
- The prior review's only finding (vacuous loop in the runtime-helper test) was fixed in 93ef252 and the thread is resolved.
- The PR description reports all 43 existing metafile tests and 12 esbuild-compat metafile tests pass; the existing dynamic-import chunk-path test in this file would catch the chunk-ref regression I checked for.
- #34531 is noted as a related workaround at the markdown layer; this PR fixes the JSON source, which is the right layer per the repo's "fix at the layer that owns the invariant" rule. The two don't conflict — they touch different code paths.
- No CODEOWNERS entry for
src/bundler/.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the runtime-helper test is now a positive toEqual(["bun:wrap"]) assertion, which closes out the earlier nit.
Checked that dynamic-import chunk references still fall through to record.path.text: computeCrossChunkDependencies.rs:178 sets source_index = Index::INVALID when rewriting the record to a chunk unique_key, so the new is_valid() guard skips them and breakOutputIntoPieces resolves the placeholder as before (covered by the existing dynamic-import test). Externals likewise have no valid source_index and keep their specifier. The original comparison was updated in lockstep so the specifier is now emitted for every deduped importer.
On the comment-cop flag at line 298: the 3-line comment explains why record.path.text is unreliable rather than what the code does, and the fix isn't a workaround — it reads the import path from the same sources[idx].path.pretty that produces the top-level inputs keys, i.e. the single source of truth. Trimming it wouldn't hurt, but it's not blocking.
Extended reasoning...
Overview
Two files touched: ~19 lines of logic in src/bundler/linker_context/MetafileBuilder.rs (the generate inputs-section emitter) and three new tests in test/bundler/metafile.test.ts. The change replaces record.path.text with sources[record.source_index].path.pretty for bundled imports so every metafile "path" string is byte-identical to its corresponding "inputs" key, and updates the "original" comparison to use the same value.
Security risks
None. This is output-formatting for a build-time diagnostic JSON; no untrusted input, no auth, no I/O beyond what the bundler already does.
Level of scrutiny
Low-medium. The metafile is diagnostic output, not on the runtime hot path, and the change is a read-only lookup into an array that's already indexed elsewhere in the same function with the same bounds pattern. The main correctness question was whether the new lookup would clobber chunk-reference unique_keys for code-split dynamic imports; I verified computeCrossChunkDependencies sets source_index = Index::INVALID on those records before this code runs, so they fall through unchanged, and the pre-existing metafile tracks dynamic-import imports test (which asserts ^\.\/chunk-[a-z0-9]+\.js$) still passes per the PR evidence.
Other factors
- My prior review flagged the runtime-helper test as vacuous; 93ef252 tightened it to
expect(importPaths).toEqual(["bun:wrap"]), which is exactly what was asked. That thread is resolved. - The RUNTIME guard is correct: parser-injected
bun:wraprecords havesource_index == Index::RUNTIMEand fall through torecord.path.text, preserving the existing output. The runtime source's pretty path ("runtime") is never leaked. - The bounds check (
idx < sources.len()) and empty-pretty fallback mirror the defensive pattern used a few lines earlier for the top-level input keys. - The comment-cop bot flagged the 3-line explanatory comment as a "paragraph-long workaround justification". I read it as a legitimate why-comment: it names the specific resolver behavior (dedup sets
source_indexwithout rewritingpath) that makesrecord.path.textthe wrong field here. The fix itself is not a workaround — it derives the import path from the exact same field (path.pretty) that produces the input keys, which is one-source-of-truth, not symptom-patching. Could be one line shorter, but not a blocker. - Evidence shows all 43 existing metafile tests + 12 esbuild-compat tests pass; the two new tests fail on the unfixed ASAN build (where parse ordering is perturbed) and pass with the fix.
- Coordination with #34531 was addressed: that PR patches
generate_markdowndownstream; this one removes the mismatch at the JSON source. No file-level conflict.
Repro
Whichever
mid*.jsreaches the resolver first gets the absolute path; every other importer keeps the raw specifier. Which one wins varies with parse-task completion order, so two consecutive builds produce different metafiles. Neither form matches the string used as the key in"inputs"(src/b/shared.js), sometafile.inputs[importPath]never resolves and the--metafile-mdreverse-dependency map misses most edges (it shows 1 importer ofshared.jsinstead of 30).Cause
resolve_import_recordshas a dedup path that, on apath_to_source_index_maphit, setsrecord.source_indexand continues without rewritingrecord.path. The "first" importer falls through to the fresh-resolve branch and does get its path rewritten; every subsequent one keeps the original specifier. The metafile emitter then writesrecord.path.textunconditionally.Fix
For bundled imports (
record.source_indexvalid), emitsources[source_index].path.pretty, which is exactly the string used as the"inputs"key. Externals and chunk references (source_indexinvalid) keeprecord.path.text.After:
This matches esbuild's metafile format, is stable across runs, and lets the markdown report's reverse-dependency lookup hit the exact-match path for every edge.
Tests
Two new cases in
test/bundler/metafile.test.ts:pathmust equal the"inputs"key and carry"original": "./b/shared.js".Both fail on current main and pass with this change. All 43 existing metafile tests and the 12 esbuild-compat metafile tests pass.
[review] gate passed · iteration 2 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file