Repository navigation
Conversation
A shared (non-entry-point) split chunk has entry_point_id=0 regardless of which files it contains, so deriving [name] from output_paths[entry_point_id] yielded the first entrypoint's basename. Reordering entrypoints changed the emitted chunk filename. For chunks with is_entry_point()==false, leave [name] as the literal "chunk" preset by PathTemplate::CHUNK (matching esbuild), and skip the output-path-derived [dir] lookup. Entry-point chunks (user-specified and dynamic-import) are unaffected. Also fix the "Multiple files share the same output path" hint: shared split chunks were labelled "asset naming" when they use the chunk template.
WalkthroughChangesChunk naming behavior
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 11:51 AM PT - Jul 29th, 2026
✅ @robobun, your commit b30e0c959ad0b2c5be1db56e966b6a59055cee38 passed in 🧪 To try this PR locally: bunx bun-pr 36326That installs a local version of the PR into your bun-36326 --bun |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/bundler/bundler_naming.test.ts`:
- Around line 306-346: The existing shared-chunk naming test only covers the
[name] contract. Extend the bundler naming coverage with a shared-chunk case
using [dir] to verify its fallback behavior, and add an assertion that duplicate
output paths produce the updated “chunk naming” diagnostic. Cover both
entry-point ordering variants consistently with the existing
ChunkNamePlaceholderSharedChunk tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 743a02f3-3d25-4d56-baaf-9748d52d6c29
📒 Files selected for processing (3)
src/bundler/linker_context/computeChunks.rssrc/bundler/linker_context/generateChunksInParallel.rstest/bundler/bundler_naming.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — my earlier ask for a collision-hint test was addressed by naming/ChunkNamingCollisionHint in 972a7fd.
What was reviewed:
- Confirmed
PathTemplate::CHUNKpresetsname: b"chunk"/dir: b"", so skipping theoutput_paths[0]lookup for non-entry chunks falls back correctly. - The removed
asset_namingbucket was always mislabeled — this loop only sees JS/CSS/HTML chunks fromcompute_chunks, never file-loader assets (those go throughadditional_output_files). onAfterApiBundleruns on the failed-build path inexpectBundled.ts, so the collision-hint assertions execute.
Extended reasoning...
Overview
Two-line bundler fix plus a diagnostic cleanup. In computeChunks.rs, the [name]/[dir] placeholder derivation from output_paths[entry_point_id] is now gated on chunk.entry_point.is_entry_point(). Shared split chunks are constructed with EntryPoint::new(source_index, 0, false, false) — a zeroed entry_point_id and is_entry_point = false — so the old unconditional lookup always hit output_paths[0], naming every shared chunk after whichever entrypoint the user listed first. Skipping the assignment leaves PathTemplate::CHUNK's preset name: b"chunk" / dir: b"" in place, matching esbuild. In generateChunksInParallel.rs, the duplicate-output-path hint's asset_naming bucket is folded into chunk_naming; the loop iterates chunks from compute_chunks which only ever contains JS/CSS/HTML chunks (file-loader assets flow through additional_output_files), so the old asset naming is '...' label was always pointing at the wrong CLI option.
Security risks
None. Pure bundler output-path/diagnostic-text logic; no untrusted input handling, allocation sizing, or lifetime changes.
Level of scrutiny
Moderate — user-visible bundler output paths — but the change is a two-branch conditional gate with a clear root cause, esbuild parity, and three targeted tests. Entry-point chunks (user-specified and dynamic-import, both is_entry_point() == true) are unaffected; verified the dynamic-import case still gets its source-derived name via the lazy- assertion in the new tests. The CHUNK_WITH_TARGET template also presets name: b"chunk", so the has_server_html_imports branch behaves the same.
Other factors
My earlier inline finding (missing test for the asset naming → chunk naming reclassification) was addressed in 972a7fd with naming/ChunkNamingCollisionHint; I checked expectBundled.ts:1279 to confirm onAfterApiBundle runs after the catch block on build failure, so those assertions on build.logs actually execute. The two ordering-variant tests assert exact filename shape (/^chunk-[a-z0-9]+\.js$/) and run the bundle end-to-end. All prior review threads are resolved. PR author reports the splitting/compile-splitting/metafile suites pass unchanged.
There was a problem hiding this comment.
LGTM — the collision-hint test I asked for earlier is now in place (naming/ChunkNamingCollisionHint), and the core [name] fix is well-covered by both entrypoint orderings.
What was reviewed:
- Confirmed
PathTemplate::CHUNK/CHUNK_WITH_TARGETpresetplaceholder.name = b"chunk"anddir = b"", so skipping theoutput_paths[0]lookup for non-entry chunks falls back to the intended literals. - The removed
asset_namingbucket was unreachable with correct semantics —chunksonly ever holds JS/CSS/HTML chunks (assets go throughadditional_output_files), so shared split chunks were being mislabelled. onAfterApiBundleruns unconditionally in the harness (even on build failure), so the note assertion in the collision test executes.
Extended reasoning...
Overview
This PR fixes a bundler bug where shared (non-entry-point) split chunks resolved the [name] placeholder to output_paths[entry_point_id()], which for shared chunks is a zeroed index — so every shared chunk was named after whichever entrypoint happened to be listed first. The fix gates the output_paths lookup on chunk.entry_point.is_entry_point(); when false, placeholder.name and placeholder.dir stay at the PathTemplate::CHUNK presets ("chunk" and ""), matching esbuild. It also corrects the "Multiple files share the same output path" hint to say chunk naming (not asset naming) for colliding shared chunks, since those chunks use the chunk template — the asset_naming branch was dead in practice because assets flow through additional_output_files, not the chunk list.
Three files touched: computeChunks.rs (~10 lines, the gate), generateChunksInParallel.rs (~15 lines, hint reclassification + dead-branch removal), and three new tests in bundler_naming.test.ts.
Security risks
None. This is bundler output-path naming logic with no untrusted-input parsing, no auth/crypto, no filesystem writes beyond what the bundler already does.
Level of scrutiny
Medium — bundler code is production-critical, but this change is narrow: a single conditional gate on an existing predicate, plus a diagnostic-text correction. The mechanism is clearly explained (zeroed entry_point_id on shared chunks indexing into output_paths[0]), and I verified against src/bundler/options.rs that the fallback presets are exactly what the PR claims. The evidence block shows all three new tests fail on main (both ASAN and release) and pass here, and the author ran the adjacent splitting/metafile suites without regression.
Other factors
- My earlier inline comment asking for a test of the
chunk naminghint was addressed in commit 972a7fd (naming/ChunkNamingCollisionHint), which usesbackend: "api"+onAfterApiBundleto assert onbuild.logs. I checkedexpectBundled.ts:1279—onAfterApiBundleruns unconditionally afterBun.build, so the assertion executes on the failure path. - The comment-cop bot flagged verbose comments twice; both were trimmed (commits e8420ff, 1f7bf8a) and threads resolved.
- CodeRabbit's request for
[dir]coverage was reasonably declined: shared chunks have no source path to derive[dir]from, andsplitting/ChunkPathDirPlaceholderImplicitOutbasealready covers the dynamic-import (entry-point) case. - Entry-point chunks (user-specified and dynamic-import) are unaffected — the gate only changes behavior for
is_entry_point() == false. - The two-ordering test (
MainFirst/ModnameFirst) directly proves the fix's stated invariant: output is stable across entrypoint order.
|
CI status: the diff is green on every lane that ran it. Build #85298 on b30e0c9 (with the Ready to merge once the naming choice in the comment above is confirmed. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Instead, why not:
- if chunk belongs to 1 entry point, [name] === [name]-chunk
- if chunk belongs to > 1 entry points, [name] === shared
…for more For a non-entry split chunk, resolve [name] from the chunk's entry_bits: a single set bit means the chunk belongs to exactly one entry point and is named "<entry-basename>-chunk"; more than one yields "shared". The chunking algorithm only produces non-entry chunks for files reachable from >=2 entries, so the single-owner branch is presently unreachable but kept for if that ever changes.
|
Done in 8bf0348. One finding: with the current chunking, a non-entry split chunk is only ever created when a file is reachable from two or more entry points (single-owner code is inlined into that entry's own chunk), so Both stable across entry order. Kept the If you'd rather the single-owner test count only user-specified entries (so a chunk shared between |
robobun
changed the title
There was a problem hiding this comment.
My earlier note on the collision-hint test is addressed — naming/ChunkNamingCollisionHint now locks in chunk naming is and rules out asset naming is.
Deferring to a maintainer for the user-facing naming choice: the author left an open question on whether single-owner should count only user-specified entries (main-chunk vs shared), and the <entry>-chunk branch is currently unreachable per the author's own analysis. Worth a quick sign-off on shared vs esbuild's chunk and whether to keep the dead branch.
Extended reasoning...
Overview
Two bundler files and one test file. computeChunks.rs gates the output_paths[entry_point_id] lookup on is_entry_point() and, for non-entry split chunks, derives [name] from entry_bits (<entry>-chunk when one bit is set, shared otherwise) and skips the [dir] derivation. generateChunksInParallel.rs drops the asset_naming bucket from the duplicate-output-path hint and folds non-user-specified-entry chunks into chunk_naming. Three new itBundled cases cover both entry orderings and the reworded hint.
Security risks
None. Pure output-filename derivation and diagnostic wording; no untrusted-input parsing, no path traversal surface introduced (the [dir] derivation is removed for shared chunks, not retargeted).
Level of scrutiny
Medium. The root-cause fix (stop reading output_paths[0] for non-entry chunks) is mechanical and clearly correct — entry_point_id is zeroed for those chunks, so the old lookup was always wrong. What remains is a user-facing API-surface decision: what [name] should expand to for shared split chunks. That is a design call, not a bug fix.
Other factors
- The
entry_bits.count() == 1→find_first_set().unwrap()path is panic-safe (count guards it) and the resulting index is bounded byentry_points.len(), sooutput_paths[ep]is in range. But per the author's own follow-up, non-entry chunks are only created when a file is reachable from ≥2 entries, so this branch never fires today. REVIEW.md flags added-but-dead code; a maintainer should decide whether to keep it speculative or collapse to just"shared". - The author explicitly asks whether single-owner should count only user-specified entries — an open design question with no maintainer reply yet.
- esbuild (the reference implementation for this bundler) uses the literal
chunkfor shared-chunk[name];sharedis a divergence a maintainer should ack. - My previous concern (untested collision-hint reclassification) is resolved by
naming/ChunkNamingCollisionHintin 972a7fd; verifiedonAfterApiBundleruns before the harness's error-log processing, so the assertion executes even on a failed build. - CI is green on all lanes that ran; no bundler test regressed.
…nk-name-placeholder
There was a problem hiding this comment.
I didn't find any correctness issues, but this changes what [name] resolves to for shared split chunks (a user-visible output-filename contract) and the last comment on the thread leaves an open question for a maintainer about whether the single-owner check should count only user-specified entries. Worth a human sign-off on the naming choice (shared / <entry>-chunk) before merging.
What was reviewed:
entry_bits.count()/find_first_set()on non-entry chunks — count is always ≥1 (files reach the split-chunk map only via a set entry bit), so the.unwrap()is safe.[dir]skip for non-entry chunks — falls through toplaceholder.dir's empty default, whichPathTemplate::printrenders as.; parallel to the[name]fix.- Collision-hint reclassification — the removed
asset_namingbucket only ever held JS split chunks (asset files areadditional_output_files, not iterated here);naming/ChunkNamingCollisionHintcovers it viaonAfterApiBundle, which runs beforebundleErrorsvalidation.
Extended reasoning...
Overview
Two Rust changes in the bundler's linker plus three new bundler tests. computeChunks.rs stops deriving [name]/[dir] for non-entry-point split chunks from output_paths[entry_point_id] (which was always index 0 because non-entry chunks are constructed with a zeroed entry_point_id), and instead sets [name] to "<entry>-chunk" when entry_bits.count() == 1 or "shared" otherwise, and skips the [dir] derivation entirely. generateChunksInParallel.rs folds the collision-hint's asset_naming bucket into chunk_naming — non-entry JS chunks use the chunk template, so hinting "asset naming" was misleading.
Security risks
None. This is output-filename derivation for the bundler; no untrusted input parsing, auth, or filesystem-escape surface is touched. The [dir] block that opens directories on disk is now gated more narrowly than before (only for entry-point chunks).
Level of scrutiny
Medium. The code path is not memory-unsafe (no new raw-pointer or FFI logic), but it does change a user-visible output contract: anyone using naming.chunk with [name] will see different filenames for shared chunks. That's the intent (the old behavior was order-dependent and clearly a bug), but the specific replacement string — "shared" vs "chunk" (esbuild's choice) vs the <entry>-chunk variant — is an API decision. The author's final comment explicitly leaves a variant of that decision open ("If you'd rather the single-owner test count only user-specified entries … just say the word"), which is a signal a maintainer should confirm the naming before this ships.
Other factors
- My prior review comment (missing test for the
chunk naminghint) was addressed bynaming/ChunkNamingCollisionHint; I verified inexpectBundled.tsthatonAfterApiBundleruns unconditionally (before thebundleErrorsfilter), so the note-text assertions are live even on a failed build. - The
.unwrap()onfind_first_set()is guarded bycount() == 1;AutoBitSet::count/find_first_setare consistent (both forward to the same variant), and non-entry chunks are keyed by a file'sentry_bitswhich by construction has ≥1 bit set for any reachable file. - The
[dir]behavior change (shared chunks now render[dir]as.instead of the first entrypoint's directory) is a parallel fix to the[name]one and was discussed/accepted in the CodeRabbit thread;splitting/ChunkPathDirPlaceholderImplicitOutbasestill covers[dir]for dynamic-import chunks. - CI is green on all lanes that ran; no bundler test failed. All bot review threads are resolved. No CODEOWNERS on these paths.
Deferring solely so a human can bless the user-facing naming string, per the author's open question.
|
Closing this PR because it has been inactive for more than 90 days. |
|
The stale bot closed this PR. #47346 replaces it and implements the rule from the review above. A chunk that one file of One correction to my earlier comment here. I wrote that |
What
With
splitting: trueand a chunk naming template containing[name], every shared (non-entry-point) chunk was named after the first entrypoint's basename instead of anything related to the chunk's own contents. Reordering entrypoints changed the emitted filename for the same chunk.Swap the entrypoint order and the same shared chunk becomes
chunks/modname-<hash>.js.Why
computeChunksderived the[name]placeholder fromoutput_paths[chunk.entry_point.entry_point_id()]for every chunk. Shared split chunks are created withis_entry_point = falseand a zeroedentry_point_id, so the lookup always hitoutput_paths[0].Fix
For chunks where
is_entry_point()is false, derive[name]from the chunk'sentry_bits:"<entry-basename>-chunk"when exactly one entry point reaches it, or"shared"when more than one does. In practice the chunking algorithm only produces non-entry chunks for files reachable from two or more entries, so today these resolve toshared. Entry-point chunks (user-specified and dynamic-import) keep their source-derived names, and the output-path-derived[dir]lookup is skipped for non-entry chunks.Also fixes the "Multiple files share the same output path" hint, which labelled colliding shared chunks as
asset naming is '...'when they use the chunk template; it now sayschunk naming is '...'.After
Output is now identical regardless of entrypoint order.
How did you verify your code works?
New tests in
test/bundler/bundler_naming.test.ts:naming/ChunkNamePlaceholderSharedChunk{MainFirst,ModnameFirst}: both entry orderings emitshared-<hash>.jsfor shared chunks andlazy-<hash>.jsfor the dynamic import, and the bundle runs.naming/ChunkNamingCollisionHint: a hash-less chunk template that collides on shared chunks produces achunk naming is '...'note, notasset naming is '...'.All three fail on main and pass with this change. Also ran
test/bundler/esbuild/splitting.test.ts,test/bundler/bundler_splitting.test.ts,test/bundler/bundler_compile_splitting.test.tsandtest/bundler/metafile.test.tswith no regressions.[review] gate passed · iteration 6 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 1 rejected · iteration 6
evidence per changed file