Repository navigation
Revert "bundler(splitting): files run in different orders do not share a chunk" - #44891
Conversation
…e a chun…" This reverts commit e879366.
|
Causes startup time hit + more chunks + undoes --min-chunk-size impact |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline note, I confirmed this is a byte-exact revert: git diff e8793665^ HEAD is empty, so the tree matches the state before #44820 landed. I also checked that the removed chunk_of_file guard in findAllImportedPartsInJSOrder.rs is safe to drop — with the run-number suffix gone, the JS chunk key is again just the entry bits, so two JS chunks cannot share a key.
Extended reasoning...
The change reverts the evaluation-order chunk-splitting pass across src/bundler (Chunk.rs, lib.rs, computeChunks.rs, findAllImportedPartsInJSOrder.rs, mergeSmallChunks.rs, deleted splitChunksByEvaluationOrder.rs), its docs, and its 325 lines of tests; it touches no security-sensitive surface. HEAD is byte-identical to the parent of the reverted commit, which rules out partial-revert inconsistencies, and the only open finding is the nit about dropping tests that still pass on the pre-feature tree.
| @@ -2346,331 +2346,6 @@ describe("bundler", () => { | |||
| run: { file: "/out/entry.js", stdout: "late2 b late1 b" }, | |||
| }); | |||
|
|
|||
There was a problem hiding this comment.
🟡 nit (optional): Maintainers lose regression coverage for nine splitting behaviors that still hold on this branch, with no reason stated for the deletion. The revert removes all 20 splitting/SharedFilesIn* tests after test/bundler/bundler_splitting.test.ts:2347, but nine of them assert pre-feature behavior and were reported passing on the pre-feature tree, which HEAD is byte-identical to. Fix: keep every deleted test that passes without the feature (restore those nine with the chunkWith helper they share), or state in the PR why each is dropped. [also at: test/bundler/bundler_splitting.test.ts:2347 - Users bundling with splitting: true and several entry points get shared side-effect modules evaluated in the wrong order again, which the base branch gets right.]
Why this was flagged
The branch is a pure revert: HEAD equals b000af7, the parent of e879366 (git diff e879366^ HEAD is empty). The feature commit's message lists 20 new tests and says 9 of them pass on the pre-feature main (one order, declarations only, files already loaded, an import cycle, entry points that import each other, the imports of an entry point's chunk, a top-level await, an import of a CommonJS module, a require() at load); all nine are deleted from test/bundler/bundler_splitting.test.ts between lines 2347 and 2349. Their helpers jsFilesIn (line 354) and launchHashedEntry (line 511) still exist, so restoring them needs only the deleted chunkWith helper. The PR description is only 'Reverts #44820', so the deletion of tests that still pass is unexplained. On the base branch these behaviors are guarded; after merging a future change to merge_small_chunks or find_all_imported_parts_in_js_order can break cycle, TLA, CJS-wrapper or require-at-load ordering under splitting without any test failing.
Verification: git diff e8793665 HEAD -- test/bundler/bundler_splitting.test.ts is 325 pure deletions after line 2347, removing 20 splitting/SharedFilesIn* tests. The feature commit e879366's own message states that 9 of them pass on the pre-feature main, and HEAD is byte-identical to that tree (git diff e8793665^ HEAD empty). The PR description is solely "Reverts #44820".
Reverts #44820