Repository navigation
fix(concatenation): keep wrapped module names and side effects intact - #21593
Merged
Merged
Conversation
A wrapped CommonJS body is emitted verbatim and never scope-analysed, so its identifiers were unknown when the concatenation allocated its own top-level names. A local declaration could shadow the wrapper accessor it calls, and a free global could be captured by another member's top-level binding. Claim the body's identifiers instead, in place of the scope analysis that is skipped. An unused export of a wrapped module also collapsed to `undefined`, dropping the accessor call with it — and with it the module's side effects, which a hoisted body would still have run at its own slot.
|
| Name | Type |
|---|---|
| webpack | Minor |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
Contributor
|
This PR is packaged and the instant preview is available (c593dea). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@c593dea
yarn add -D webpack@https://pkg.pr.new/webpack@c593dea
pnpm add -D webpack@https://pkg.pr.new/webpack@c593dea |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #21593 +/- ##
==========================================
- Coverage 94.09% 94.08% -0.01%
==========================================
Files 627 627
Lines 77209 77229 +20
Branches 22656 22660 +4
==========================================
+ Hits 72646 72663 +17
- Misses 4563 4566 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Coverage showed the WeakMap around the identifier scan never hit: each _analyseModule builds a fresh Source, so the key is always new. It cost an allocation and a lookup per wrapped body for nothing. The unused-export change at the second getFinalBinding site is reverted: no build reaches it, because an unused ESM re-export of a wrapped module is dropped before code generation. The require()-shaped site keeps the fix and is the one a build actually hits.
Merging this PR will improve performance by 40.93%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | benchmark "many-modules-interop-runtime", scenario '{"name":"mode-production","mode":"production"}', measure 'exec' |
24.6 KB | 17.5 KB | +40.93% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/concatenate-wrapped-module-names (ef2a86b) with main (f2462c2)
ConfigTestCases and ConfigCacheTestCases run the same cases, and a worker that shares one global between test files (Bun's --workerThreads) carries their leftovers into the second run. The harness only clears globals whose name contains "webpack", so a plain counter survives: the deferred-evaluation case then saw a non-zero count and the inline-exports case saw its side effects twice. Both now clear their own counter. The amd-runtimeChunk library variants are a timing difference, not a leftover: the `promise` external is still settling when the harness deletes the chunk loading globals, which only survives because Node gives each test file a fresh global. Filtered out under Bun until the harness can await those externals.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three correctness bugs in the wrapped-CommonJS concatenation added in #21519, found by compiling the same fixtures under
concatenateModules: false,{ commonjs: false }and{ commonjs: true }and diffing the runtime behaviour. Refs #21519.A wrapped body is emitted verbatim and never scope-analysed, so its identifiers never entered
allUsedNames: a local declaration could shadow the wrapper accessor it calls (var dep_namespaceFn = …next torequire("./dep.js")throwsdep_namespaceFn is not a function, and thefunctionform fails silently), and a free global could be captured by another member's top-level binding.ConcatenatedModulenow claims those identifiers with a cached lexical scan, in place of the scope analysis that is deliberately skipped. Separately, an unused export of a wrapped module collapsed toundefined, dropping the accessor call and with it the module's side effects — a three-way require cycle evaluated[a, b]instead of[a, b, c]; it now renders as(x_namespaceFn(), undefined).The code asserted an invariant that was never implemented ("the reserved
__webpack_prefix is what keeps a local in that body from shadowing the accessor" —findNewNameproducesdep_namespaceFn); those comments are corrected.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes — four
test/configCases/concatenate-modules/cases:commonjs-wrapped-name-collision,commonjs-wrapped-global-capture,commonjs-unused-export-side-effects, andcommonjs-require-shapes(template-literal, destructured,this-binding, computed-request, alias and bothnewrequire forms). Each was verified to fail with thelib/change reverted.Does this PR introduce a breaking change?
No — it only corrects output for modules wrapped by the opt-in
optimization.concatenateModules.commonjspath.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
Yes. Claude Code was used to build the differential harness that surfaced the three divergences, to write the fix and the test cases, and to verify each test fails without the fix. Every finding was reproduced in a real build and reviewed by me before committing.
Generated by Claude Code
Note
Medium Risk
Changes affect production bundle codegen on the opt-in CommonJS concatenation path in
ConcatenatedModule.js; incorrect identifier reservation or unused-export expressions could alter runtime behavior, though the added config cases target the reported failure modes.Overview
Fixes three correctness bugs in opt-in
optimization.concatenateModules.commonjswrapped-module output: verbatim CJS bodies were never scope-analyzed, so generated names could collide with body identifiers and unused exports could skip lazy accessor evaluation.Identifier reservation: Adds
findIdentifiersand, during the RESERVE phase, registers every identifier in a wrapped body inallUsedNamessofindNewNamedoes not pick names likedep_namespaceFnthat locals or hoisted declarations inside the body would shadow.Unused wrapped exports: Adds
getUnusedExportExprso unused exports on wrapped modules render as(accessor(), undefined)instead of bareundefined, preserving evaluation and side effects when nothing reads the export.Tests: Four
concatenate-modulesconfig cases cover name collision, global capture, unused-export side effects, andrequire()shapes; smaller harness tweaks clear shared globals on Bun and skip a flaky library case there.Reviewed by Cursor Bugbot for commit ef2a86b. Bugbot is set up for automated code reviews on this repo. Configure here.