module: CJS / ESM resolver sharing and refactoring#34744
Closed
guybedford wants to merge 2 commits into
Closed
Conversation
nodejs-github-bot
added
errors
Collaborator
|
Review requested:
|
guybedford
changed the title
Member
|
Wait, so now you can map your exports to things in node_modules?? Can you help me understand why that's a good idea? |
Contributor
Author
|
Sure, I can add it back, it's just that the spec was actually incorrect since it was only checking the package.json target path, when we in fact need to check both the target path and the user subpath. We give separate errors for these two cases because the one is package author issue while the other is a package consumer issue. This error separation doesn't apply as easily for patterns though so those errors may need to be rethought. I refactored it out because it seems overly strict and unnecessary but I can add it back. |
Member
|
I think it's very critical that "exports" only be possible to use to remap an entry specifier to "a file within the package that has exports". |
hybrist
approved these changes
Aug 12, 2020
Contributor
Author
|
@ljharb I've added back the node_modules restriction identically to previously with a refactoring to make it part of the new segment check approach. |
ljharb
reviewed
Aug 12, 2020
guybedford
force-pushed
the
resolve-refactor
branch
from
734865c to
fc1f44b
Compare
Contributor
Author
|
I would like to land this PR soon. Any further feedback welcome. |
Contributor
Author
|
@nodejs/modules-active-members for any further review |
Collaborator
jasnell
approved these changes
Aug 12, 2020
Collaborator
guybedford
added a commit
that referenced
this pull request
Aug 13, 2020
PR-URL: #34744 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Contributor
Author
|
Landed in f8976a7. |
addaleax
added a commit
to addaleax/node
that referenced
this pull request
Aug 13, 2020
Broken locally for me since f8976a7. Refs: nodejs#34744
3 tasks
MylesBorins
pushed a commit
that referenced
this pull request
Aug 17, 2020
PR-URL: #34744 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
BethGriggs
pushed a commit
that referenced
this pull request
Aug 20, 2020
PR-URL: #34744 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
guybedford
added a commit
to guybedford/node
that referenced
this pull request
Sep 28, 2020
PR-URL: nodejs#34744 Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Merged
Han5991
added a commit
to Han5991/node
that referenced
this pull request
May 23, 2026
Previously, `requireStack` was attached only on the bottom fallback path of `Module._resolveFilename`. Errors thrown from `tryPackage`, `trySelf`, the `#imports` branch, `finalizeEsmResolution`, and from within `Module._findPath` (exports/imports resolution failures) all escaped without it, leaving the diagnostic uneven depending on which inner path failed. This change makes the diagnostic uniform across those paths via two helpers: * `buildRequireStack(parent)` walks the parent chain. It is called lazily on the error path only, so successful resolves keep their original cost and a pathological parent-chain cycle from `module.parent = module` cannot hang the hot path. * `attachRequireStack(err, parent)` applies the stack to an error when (a) the error code is in a `SafeSet` of module-resolution codes (`MODULE_NOT_FOUND`, `ERR_MODULE_NOT_FOUND`, `ERR_PACKAGE_PATH_NOT_EXPORTED`, `ERR_PACKAGE_IMPORT_NOT_DEFINED`, `ERR_INVALID_MODULE_SPECIFIER`, `ERR_INVALID_PACKAGE_TARGET`, `ERR_INVALID_PACKAGE_CONFIG`), (b) `err.requireStack` is not already an array, and (c) the error object is extensible. The last check protects against monkey-patched `Module._findPath` implementations that may throw frozen errors. The catch blocks in the `#imports`, `trySelf`, and `Module._findPath` paths delegate to `attachRequireStack`, and the bottom fallback uses `buildRequireStack` directly. Resolves two long-standing `TODO(BridgeAR): Add the requireStack as well` comments in `lib/internal/modules/cjs/loader.js`, originally deferred in nodejs#26823 (2019) and carried through nodejs#34744 (2020). Tests cover the `#imports`-to-missing-file case, the `ERR_PACKAGE_PATH_NOT_EXPORTED` case via self-require, a frozen- error monkey-patch (must not crash), and a preset non-array `requireStack` value (must be replaced). Existing assertions in `test-module-loading.js` and `test-module-loading-error.js` are extended to verify the new property on errors that previously lacked it. Signed-off-by: sangwook <rewq5991@gmail.com>
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
6 participants
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.
This extracts just the resolver refactoring component from #34718.
I hope that the negative diff speaks for itself at least!
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes