fix(esm): deconflict external bindings in module output#13466
fix(esm): deconflict external bindings in module output#13466JSerFeng merged 9 commits intoweb-infra-dev:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a04346928d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Fixes duplicate-identifier hazards in ESM library output by deconflicting external binding names (including namespace locals) during linking/rendering, especially when the same external target is reached via mixed module-import and node-commonjs paths.
Changes:
- Update ESM library linking/name-allocation logic to avoid collisions between external top-level bindings and namespace import locals.
- Add new externals regression cases (namespace+naming mixes, cross-module collisions, mixed-target
webpack-sourcesrepro). - Refresh affected ESM output snapshots to reflect new deconflicted bindings and
export * as ...emission.
Reviewed changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rspack-test/esmOutputCases/externals/treeshake-namespace-access/snapshots/esm.snap.txt | Snapshot updated to reflect namespace-based external access naming (import * as ...). |
| tests/rspack-test/esmOutputCases/externals/runtime-decide/snapshots/esm.snap.txt | Snapshot updated to reflect deconflicted require local (runtime_decide_0). |
| tests/rspack-test/esmOutputCases/externals/reexport-imported-namespace-via-module/snapshots/esm.snap.txt | Snapshot updated to emit export * as ... from for namespace reexport. |
| tests/rspack-test/esmOutputCases/externals/reexport-external-namespace-as/snapshots/esm.snap.txt | Snapshot updated to emit multiple export * as ... from reexports. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-name-collision-across-modules/rspack.config.js | New regression config for cross-module name collisions with node-commonjs externals. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-name-collision-across-modules/path-ns.js | New regression module provoking namespace-name collisions across modules. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-name-collision-across-modules/named.js | New regression module reexporting named fs bindings. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-name-collision-across-modules/index.js | New regression test validating deconfliction behavior at runtime. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-name-collision-across-modules/snapshots/esm.snap.txt | New snapshot asserting final deconflicted output for the regression. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-and-named/rspack.config.js | New regression config for mixing namespace and named imports with node-commonjs. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-and-named/namespace.js | New regression module importing a namespace from fs. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-and-named/named.js | New regression module reexporting named bindings from fs. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-and-named/index.js | New regression test checking namespace/named alignment. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-and-named/snapshots/esm.snap.txt | New snapshot asserting expected ESM output for the regression. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-import-namespace-reexport-with-named/rspack.config.js | New regression config for importing and reexporting namespace with named exports (node-commonjs). |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-import-namespace-reexport-with-named/index.js | New runtime regression test for combined namespace + named reexports. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-import-namespace-reexport-with-named/snapshots/esm.snap.txt | New snapshot validating rewritten exports for the regression. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-export-namespace-as-with-named/rspack.config.js | New regression config for export * as plus named reexports (node-commonjs). |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-export-namespace-as-with-named/index.js | New runtime regression test for export * as + named exports. |
| tests/rspack-test/esmOutputCases/externals/node-commonjs-export-namespace-as-with-named/snapshots/esm.snap.txt | New snapshot validating combined export * as and named exports output. |
| tests/rspack-test/esmOutputCases/externals/namespace-name-collision-across-modules/rspack.config.js | New regression config for cross-module collisions with module-import externals. |
| tests/rspack-test/esmOutputCases/externals/namespace-name-collision-across-modules/path-ns.js | New regression module provoking namespace-name collisions across modules. |
| tests/rspack-test/esmOutputCases/externals/namespace-name-collision-across-modules/named.js | New regression module reexporting named fs bindings. |
| tests/rspack-test/esmOutputCases/externals/namespace-name-collision-across-modules/index.js | New runtime regression test validating module-import deconfliction. |
| tests/rspack-test/esmOutputCases/externals/namespace-name-collision-across-modules/snapshots/esm.snap.txt | New snapshot asserting expected multi-asset output for the regression. |
| tests/rspack-test/esmOutputCases/externals/namespace-and-named/snapshots/esm.snap.txt | Snapshot updated to use namespace binding for named external access. |
| tests/rspack-test/esmOutputCases/externals/mixed-module-import-and-node-commonjs-same-target/rspack.config.js | New regression config for mixed external types targeting the same module. |
| tests/rspack-test/esmOutputCases/externals/mixed-module-import-and-node-commonjs-same-target/namespace.js | New regression module importing namespace via module-import external. |
| tests/rspack-test/esmOutputCases/externals/mixed-module-import-and-node-commonjs-same-target/named.js | New regression module importing named exports via node-commonjs external. |
| tests/rspack-test/esmOutputCases/externals/mixed-module-import-and-node-commonjs-same-target/index.js | New runtime regression test for mixed external type equivalence and no dup identifiers. |
| tests/rspack-test/esmOutputCases/externals/mixed-module-import-and-node-commonjs-same-target/snapshots/esm.snap.txt | New snapshot validating deconflicted top-level bindings for mixed external types. |
| tests/rspack-test/esmOutputCases/externals/import-namespace-reexport/snapshots/esm.snap.txt | Snapshot updated to emit export * as ... from instead of export { ns as ... }. |
| tests/rspack-test/esmOutputCases/externals/import-namespace-reexport-with-named/snapshots/esm.snap.txt | Snapshot updated to split named export and export * as namespace export. |
| tests/rspack-test/esmOutputCases/externals/import-namespace-reexport-with-named-same-source/rspack.config.js | New regression config for namespace import + named exports from the same source. |
| tests/rspack-test/esmOutputCases/externals/import-namespace-reexport-with-named-same-source/index.js | New runtime regression test for same-source namespace + named reexports. |
| tests/rspack-test/esmOutputCases/externals/import-namespace-reexport-with-named-same-source/snapshots/esm.snap.txt | New snapshot validating combined same-source exports output. |
| tests/rspack-test/esmOutputCases/externals/export-namespace-as/snapshots/esm.snap.txt | Snapshot updated to emit export * as ... from for namespace export. |
| tests/rspack-test/esmOutputCases/externals/export-namespace-as-with-named-same-source/rspack.config.js | New regression config for export * as with named exports from same source. |
| tests/rspack-test/esmOutputCases/externals/export-namespace-as-with-named-same-source/index.js | New runtime regression test for combined exports from same source. |
| tests/rspack-test/esmOutputCases/externals/export-namespace-as-with-named-same-source/snapshots/esm.snap.txt | New snapshot validating expected export rewriting. |
| tests/rspack-test/esmOutputCases/externals/dedup-external-imports-mixed/snapshots/esm.snap.txt | Snapshot updated to reflect deconflicted require local (cjs_consumer_0). |
| crates/rspack_plugin_esm_library/src/link.rs | Core fix: broaden deconfliction set and adjust namespace import/export handling during linking/rendering. |
| crates/rspack_core/src/concatenated_module.rs | Align concatenation name allocation: track internal namespace import names to avoid collisions. |
Comments suppressed due to low confidence (2)
tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-and-named/rspack.config.js:1
- This new config fixture uses tab indentation and omits trailing commas, which is inconsistent with the surrounding test configs in this PR (2-space indentation and trailing commas). Align formatting to the prevailing style to keep test fixtures consistent and reduce noisy diffs in future edits.
tests/rspack-test/esmOutputCases/externals/node-commonjs-namespace-name-collision-across-modules/path-ns.js:1 - The local identifier
external_fs_namespaceObjectintentionally refers to thepathmodule, which is confusing without context. If this is meant to provoke a collision scenario, add a brief comment explaining the intent (or choose an equally-colliding but self-describing name) so future maintainers don’t 'fix' it and accidentally remove the coverage this case provides.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
JSerFeng
force-pushed
the
fy/fix-esm-external-name-dedup
branch
from
a043469 to
8f8c996
Compare
github-actions
bot
added
the
release: bug fix
Merging this PR will improve performance by 3.25%🎉 Hooray!
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | js@Traverse module graph by dependencies |
543.6 µs | 526.5 µs | +3.25% |
Comparing JSerFeng:fy/fix-esm-external-name-dedup (5d2a945) with main (95fbc19)
Footnotes
-
17 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
JSerFeng
force-pushed
the
fy/fix-esm-external-name-dedup
branch
from
7a3c61d to
de699ae
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4270e3fc3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Chef's kiss. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Summary
module-importandnode-commonjspathswebpack-sourcesmixed-target reproRelated links
rslibmain intopackages/rspackand building withrslibChecklist