module: add requireStack to all error paths#62059
Open
Han5991 wants to merge 3 commits into
Open
Conversation
Collaborator
|
Review requested:
|
nodejs-github-bot
added
module
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #62059 +/- ##
=======================================
Coverage 90.14% 90.15%
=======================================
Files 718 718
Lines 227984 228042 +58
Branches 42835 42841 +6
=======================================
+ Hits 205522 205589 +67
+ Misses 14235 14231 -4
+ Partials 8227 8222 -5
🚀 New features to boost your workflow:
|
Han5991
force-pushed
the
fix/cjs-loader-require-stack
branch
2 times, most recently
from
17b4a9e to
81f253b
Compare
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>
Han5991
force-pushed
the
fix/cjs-loader-require-stack
branch
from
81f253b to
9fa4631
Compare
Signed-off-by: sangwook <rewq5991@gmail.com>
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
2 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.
The CJS loader attaches
requireStackonly on the bottom fallbackpath of
Module._resolveFilename. Errors fromtryPackage,trySelf, the#importsbranch,finalizeEsmResolution, and fromwithin
Module._findPath(exports/imports resolution failures)escape without it. This PR makes the diagnostic uniform across
those paths.
Resolves the two
TODO(BridgeAR)comments deferred in #26823(2019) and carried through #34744 (2020).
The second commit on this branch addresses self-review findings:
broader module-resolution error-code filter, lazy parent-chain walk
(no hot-path cost), and an
ObjectIsExtensibleguard so theattachment cannot mask the original error from a monkey-patched
Module._findPath. Commit messages contain the full design notes.Tests in
test/parallel/test-module-require-stack-paths.jscoverthe newly attached paths; existing tests are extended where the
property was previously absent.