Repository navigation
fix(commonjs): handle node: builtins with strictRequires: auto - #1930
Merged
Merged
Conversation
Collaborator
|
Thanks for the PR. Please take some time to describe what your changes do differently. We're also discussing this in #1924 |
shellscape
requested review from
CharlieHelps
and removed request for
CharlieHelps
October 24, 2025 03:19
Contributor
Author
|
my apologies! charlie's summary in the linked issue and the code comment in this PR cover what i was intending, but i'll write in more detail when i'm back at my computer |
danielroe
commented
Oct 24, 2025
| isExternalWrapped | ||
| ) { | ||
| let resolvedDependencyId = dependencyId; | ||
| if (parentMeta.isCommonJS === IS_WRAPPED_COMMONJS && !allowProxy && isExternalWrapped) { |
Contributor
Author
There was a problem hiding this comment.
- Previously, we checked
parentMeta.initialCommonJSType === IS_WRAPPED_COMMONJSinstead ofparentMeta.isCommonJS === IS_WRAPPED_COMMONJS. With strictRequires: 'auto', I believe the initial type (before cycle detection) can differ from the final type.
danielroe
commented
Oct 24, 2025
Comment on lines
+206
to
+213
| } else if (isExternalWrapped && !allowProxy) { | ||
| // If the parent is not wrapped but the dependency is a node: builtin external, | ||
| // unwrap the EXTERNAL_SUFFIX so it's treated as a normal external. | ||
| // This avoids trying to load the lazy __require proxy for non-wrapped contexts. | ||
| const actualExternalId = unwrapId(dependencyId, EXTERNAL_SUFFIX); | ||
| if (actualExternalId.startsWith('node:')) { | ||
| resolvedDependencyId = actualExternalId; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
node:builtins with the EXTERNAL_SUFFIX were always loaded withgetExternalBuiltinRequireProxy()which only exports__require(). However, when the parent module is not wrapped (no cycles detected with strictRequires: 'auto'), the generated code tried to import a default export that didn't exist.
(so in this case we can remove the suffix)
shellscape
requested review from
CharlieHelps
and removed request for
CharlieHelps and
shellscape
October 24, 2025 13:24
Collaborator
|
|
Closed
4 of 9 tasks
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.
Rollup Plugin Name:
@rollup/plugin-commonjsThis PR contains:
Are tests included?
Breaking Changes?
If yes, then include "BREAKING CHANGES:" in the first commit message body, followed by a description of what is breaking.
List any relevant issue numbers:
resolves #1929
Description
this fixes an issue introduced in #1909, which added special handling for
node:builtins to avoid eager loading in wrapped modules...