Repository navigation
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 6 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status: rebased onto main (449877f), ready for review. Reproduced with the commands under Notes in the description.
|
79c132b to
2f918ee
Compare
… --metafile-md The "Loads these chunks" list of each entry point in the markdown metafile read only the imports of the entry point's own output file. A chunk that only another chunk imports was not in the report. Walk the import-statement edges of outputs[..].imports from the entry point's output file until no new chunk turns up, and list every chunk that loads with the entry point once. Name the importing chunk for a chunk that the output file does not import itself. The chunks that the output file loads with import() or require() follow, as before. They are not walked, because each has an entry section of its own.
An entry point takes the code that it shares with its import() targets only with [hash] in its name (#44023). The test "does not list what an imported entry point loads on demand" needs that layout: the chunk of a lazy route imports the output file of the entry point. Cast the metafile option of the helper, as the other tests in this file do. bun-types declares the option as a boolean.
b35c60a to
67746b0
Compare
|
Updated 7:20 AM PT - Oct 26th, 2026
✅ @robobun, your commit 67746b0044af4ee3ec2594eab7df343e82a2efe3 passed in 🧪 To try this PR locally: bunx bun-pr 42755That installs a local version of the PR into your bun-42755 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — a self-contained change to the markdown metafile report, with tests that pin the new output shape.
What was reviewed:
- The
loaded_chunkswalk: theseenset is seeded with the entry's own path, so cycles (a route chunk importing the entry's output back) terminate, and onlyimport-statementedges are followed, soimport()/require()targets are listed but not walked. StringHashMap::get_or_put/putsemantics match how the same file already uses them;ImportKind::Stmt.label()isb"import-statement", the kind string the metafile emits.- The writer's size branch: a path missing from
outputs(externals) still prints without a size, same as before; the retainedexpect("int cast")only sees bytes the bundler itself wrote. - Tests use
tempDir+using, drain pipes withPromise.all, assert stderr before exit code, and compare whole sections withtoEqual.
Extended reasoning...
The diff touches one function family in src/bundler/linker_context/MetafileBuilder.rs (the "Loads these chunks" section of generate_markdown, replaced by a breadth-first helper over outputs[..].imports plus an outputs-by-path index) and adds five tests in test/bundler/metafile.test.ts; meta.json and emitted bundles are untouched. It is report-generation only, with no security-sensitive surface. The change is small (~70 lines of Rust), reads data the bundler itself produced, follows the file's existing StringHashMap patterns, and no CODEOWNERS entry covers the changed paths. The bug hunt ran dry with no findings, and the new tests cover the transitive case, both lazy kinds, and the no-over-listing case.
Problem
--splitting,--metafile-mdleaves out a chunk that only another chunk imports. The repro (Notes) writes three chunks:meta.mdnames two.import "./chunk.js"that an entry point's file had for a chunk that runs nothing.generate_markdown(src/bundler/linker_context/MetafileBuilder.rs:1162) reads one level ofoutputs[entry].imports.Fix
loaded_chunkswalks theimport-statementedges ofoutputs[..].importsfrom the entry point's output file. An indirect chunk getsviaand its importer.import()orrequire()follow, as before. The walk skips them: each has its own entry section.meta.jsonand the bundles do not change.test/bundler/metafile.test.ts(five new tests, main 449877f fails four),test/bundler/esbuild/metafile.test.ts. Self-reviewed: 6 concerns raised, 4 addressed (Notes).Background
outputs[file].importslists the files thatfileimports, each with akind(import-statement,dynamic-import,require-call).--metafile-mdrenders that JSON.--splitting, shared code moves to chunks. Animport()target is a chunk with its own "Entry" section.outputs[..].imports: the JSON would list imports the file does not have.Downsides
import-statementchunks, then lazy chunks (before: JSON order). A chunk imported with two kinds is one line.meta.mdgrows where chunks were missing: 210,864 to 1,703,090 bytes for 200 entry points over 200 chained chunks (1.4.0: 1,225,641)..text+3,584 bytes, file size unchanged (size, release builds). A report with no missing chunk: same bytes, not slower (Notes).Notes
Repro:
meta.mdnames### Entry: src/m0.tsbefore:After:
Where the regression comes from:
out/m0.jshas the statementimport"./m0-q20h2fjv.js";for the chunk that holdsc3.cjs. Sooutputs["./m0.js"].importshas three entries. In 1.4.1 the statement is gone and the list has two entries.src/bundler/linker_context/computeCrossChunkDependencies.rs:522("Nothing is used from it; skip it if loading it runs nothing").git blamegives it to bundler: fold code-splitting chunks that always load together, drop dead cross-chunk imports #40506. That change is intended: the file does not need the statement. The JSON is right, and the report has to follow the edges.About the change:
Bun.build({ metafile: { markdown } })uses the same writer and has the same gap. Four of the new tests use it.Bun.buildtests shows the same gap without.cjsor--target=bun.import()orrequire(): the first version of this change walked every edge. When an entry point takes the code that it shares with its lazy routes, each route's chunk imports the entry point's output file, and that fileimport()s every route. So the section of each route listed all other routes asdynamic-import, via ./entry.js: N routes gave about N x N lines, and to load one route does not load the others. Since bundler(splitting): one edge rule for both walks; no chunk imports an unhashed entry #44023 an entry point takes that code only with[hash]innaming.entry, or when it is an HTML entry point. Each lazy chunk is an entry point in the JSON and has its own section, so the report still names every chunk.import-statementchunks before thedynamic-importandrequire-callchunks (before: JSON order, lazy first). An output that imports one chunk with two kinds now lists it once: underimport-statementif that is one of the kinds, else under the first kind in the JSON (before: one line per kind).outputsby path replaces the linearoutputs_obj.getscan per listed chunk. The walk does one lookup per edge, so the scan would be quadratic in the number of outputs.outputsis named in the report), the ESM-only shape throughBun.build, adynamic-importand arequire-callchunk (listed last, not walked), and two lazy routes that import the entry point's output file back. The last test passes before and after. It pins that the section of one route does not list the other route.Rebase onto 449877f:
test/bundler/metafile.test.ts.naming.entry = "[name]-[hash].[ext]". With a plain name, bundler(splitting): one edge rule for both walks; no chunk imports an unhashed entry #44023 givesshared.jsa chunk of its own, and no route imports the entry point's output file.metafileoption, as the other tests in the file do.bun-typesdeclares the option asboolean.outputs[..].byteswas short by the length of each cross-chunk import path. bundler: report the emitted size of each chunk as metafile outputs[..].bytes #42760 corrected that on main. The report prints the emitted sizes.Measurements (Linux x64):
sizeon release builds of 449877f and of this PR:text107,556,528 to 107,560,112..text82,164,617 to 82,168,201. No other section changes. The file is 107,730,656 bytes in both.e<i>.jsimportse<i+1>.js, every file runs code when it loads (399 outputs).meta.mdis 1,130,719 bytes in both builds.bun build --splitting --metafile-md, median of 60 interleaved runs: 128.7 ms on main, 122.8 ms with this PR. The same build without--metafile-md: 101.4 ms and 102.5 ms. For one binary, the median of the even runs and of the odd runs differ by 2 to 5 ms.e<i>.jsimportslib<i>.js,lib<i>.jsimportslib<i+1>.js, nolibfile runs code when it loads (399 outputs). main prints 200 chunk lines. This PR prints 20,099, and 19,899 of them carryvia. 1.4.0 printed 20,099.meta.md: 210,864 bytes on main, 1,703,090 with this PR, 1,225,641 on 1.4.0. Build time, median of 60 interleaved runs: 66.2 ms on main, 75.0 ms with this PR.import()routes, 300 shared modules,--target=browser(174 outputs): 435 chunk lines and 197,277 bytes on main, 444 chunk lines (9 withvia) and 198,002 bytes with this PR.valgrind,perf,bloatyandstracewere not available for these runs, so there are no instruction counts and no allocation counts. From the code: each report allocates one map of the outputs, and each entry section allocates oneVecand one set for the walk. A build without--metafile-mddoes not callgenerate_markdown.Checks:
chunk()(that chunk exists for browser targets, the tests build forbun).cargo clippy -p bun_bundler --no-deps,cargo fmt --all -- --checkandprettier --checkare clean.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/metafile.test.ts