fix(externals): correct external type for aliased node builtin externals#13627
Merged
fix(externals): correct external type for aliased node builtin externals#13627
Conversation
Refactor EsmNodeTargetPlugin from a factorize-phase ExternalsPlugin to
an after_factorize hook that corrects the external type for node builtin
externals. This fixes aliased externals like `{ 'node:fs': 'node:path' }`
where the previous factorize hook would override the user's alias by
matching first and using the original request.
The new approach:
- Uses after_factorize (stage=-10) to inspect already-created external
modules and fix their type based on dependency category
- ESM dependencies get "module-import", non-ESM get "node-commonjs"
- Runs before EsmLibraryPlugin's after_factorize (stage=0) which
depends on the correct type for set_id
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes incorrect external type handling for aliased Node builtin externals (e.g. externals: { 'node:fs': 'node:path' }) by moving the Node-builtin adjustment to an after_factorize hook and adding a way to update an ExternalModule’s external type while keeping its identifier consistent.
Changes:
- Refactor
EsmNodeTargetPluginfrom anExternalsPluginfactorize-phase approach to aNormalModuleFactory.after_factorizehook (stage -10). - Add
ExternalModule::set_external_type()to update the type and recompute the module identifier. - Add
esm-node-target-aliastest case + snapshot to verify aliased builtin requests produce correct ESM imports.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/rspack-test/esmOutputCases/externals/esm-node-target-alias/rspack.config.js | Adds a repro config that aliases a Node builtin external (node:fs → node:path). |
| tests/rspack-test/esmOutputCases/externals/esm-node-target-alias/index.js | Adds assertions ensuring the aliased request is honored and the external is emitted as ESM import. |
| tests/rspack-test/esmOutputCases/externals/esm-node-target-alias/snapshots/esm.snap.txt | Snapshot verifying output imports from node:path (aliased request), not node:fs. |
| crates/rspack_plugin_rslib/src/plugin.rs | Switches to applying the new EsmNodeTargetPlugin implementation. |
| crates/rspack_plugin_externals/src/lib.rs | Re-exports EsmNodeTargetPlugin instead of the previous factory function. |
| crates/rspack_plugin_externals/src/esm_node_target_plugin.rs | Implements new after_factorize hook to correct external type on already-created external modules. |
| crates/rspack_core/src/external_module.rs | Adds set_external_type() that recomputes the module identifier after changing external type. |
| crates/rspack_binding_api/src/raw_options/raw_builtins/mod.rs | Updates builtin plugin wiring to construct/box EsmNodeTargetPlugin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
Rsdoctor Bundle Diff AnalysisFound 6 projects in monorepo, 6 projects with changes. 📊 Quick Summary
📋 Detailed Reports (Click to expand)📁 popular-libsPath:
📁 react-10kPath:
📁 react-1kPath:
📁 react-5kPath:
📁 romePath:
📁 ui-componentsPath:
Generated by Rsdoctor GitHub Action |
Narrow the after_factorize condition to only fire when dep.category != "esm" && external_type == "module", matching the original intent. Add CJS require test case to verify the downgrade path.
Merging this PR will degrade performance by 2.63%
Performance Changes
Comparing 
Footnotes
|
Contributor
📦 Binary Size-limit
🎉 Size decreased by 6.22KB from 49.35MB to 49.34MB (⬇️0.01%) |
Timeless0911
reviewed
Apr 7, 2026
…ndencyType - Rename option from externalEsmNodeBuiltin to autoCjsNodeBuiltin - Match both "module" and "module-import" external types via starts_with - Use DependencyType::CjsRequire/CjsFullRequire instead of category check - Add module-import CJS require test case
ExternalModule::lib_ident was returning user_request (the original import specifier), which caused non-deterministic moduleIds when multiple imports alias to the same external target. Now returns request.primary() (the resolved target), making the output stable.
…ts in test - set_external_type only sets the type field without recomputing id - Revert lib_ident to use user_request (id stability to be addressed separately) - Use different external targets in test to avoid non-deterministic dedup
Timeless0911
approved these changes
Apr 8, 2026
Merged
5 tasks
2 tasks
hardfist
pushed a commit
that referenced
this pull request
Apr 9, 2026
…als (#13627) * fix(externals): correct external type for aliased node builtin externals Refactor EsmNodeTargetPlugin from a factorize-phase ExternalsPlugin to an after_factorize hook that corrects the external type for node builtin externals. This fixes aliased externals like `{ 'node:fs': 'node:path' }` where the previous factorize hook would override the user's alias by matching first and using the original request. The new approach: - Uses after_factorize (stage=-10) to inspect already-created external modules and fix their type based on dependency category - ESM dependencies get "module-import", non-ESM get "node-commonjs" - Runs before EsmLibraryPlugin's after_factorize (stage=0) which depends on the correct type for set_id * fix: only downgrade module→node-commonjs for non-ESM deps Narrow the after_factorize condition to only fire when dep.category != "esm" && external_type == "module", matching the original intent. Add CJS require test case to verify the downgrade path. * fix: add Default impl for EsmNodeTargetPlugin to satisfy clippy * refactor: rename to autoCjsNodeBuiltin, match module-import, use DependencyType - Rename option from externalEsmNodeBuiltin to autoCjsNodeBuiltin - Match both "module" and "module-import" external types via starts_with - Use DependencyType::CjsRequire/CjsFullRequire instead of category check - Add module-import CJS require test case * fix: use different external targets to avoid non-deterministic dedup * fix: use request instead of user_request for ExternalModule lib_ident ExternalModule::lib_ident was returning user_request (the original import specifier), which caused non-deterministic moduleIds when multiple imports alias to the same external target. Now returns request.primary() (the resolved target), making the output stable. * fix: simplify set_external_type, revert lib_ident, use distinct targets in test - set_external_type only sets the type field without recomputing id - Revert lib_ident to use user_request (id stability to be addressed separately) - Use different external targets in test to avoid non-deterministic dedup * test: update snapshots for existing external comment changes
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
EsmNodeTargetPluginfrom a factorize-phaseExternalsPluginto anafter_factorizehookexternals: { 'node:fs': 'node:path' }where the old factorize hook would override the user's alias by matching first (viais_node_builtin) and using the original requestset_external_type()toExternalModulewhich updates the type and recomputes the identifierThe new
after_factorize(stage=-10) inspects already-created external modules: if the request is a node builtin, it sets the correct type based on the dependency category (ESM →module-import, CJS →node-commonjs). This runs beforeEsmLibraryPlugin'safter_factorize(stage=0) which depends on the correct type forset_id.Test plan
esm-node-target-aliastest case: verifiesexternals: { 'node:fs': 'node:path' }generatesimport { resolve } from "node:path"(not"node:fs")