X Tutup
The Wayback Machine - https://web.archive.org/web/20230103071052/https://github.com/nodejs/node/pull/45734
Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added the requireStack to loader.js #45734

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Added the requireStack to loader.js #45734

wants to merge 3 commits into from
+6 −2

Conversation

mertcanaltin
Copy link

@mertcanaltin mertcanaltin commented Dec 4, 2022

// TODO(BridgeAR): Add the requireStack as well.

I also added the require stack

@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Dec 4, 2022

Review requested:

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 4, 2022
RaisinTen
RaisinTen reviewed Dec 4, 2022
View changes
lib/internal/modules/cjs/loader.js Outdated
@@ -413,7 +413,7 @@ function tryPackage(requestPath, exts, isMain, originalPath) {
err.code = 'MODULE_NOT_FOUND';
err.path = path.resolve(requestPath, 'package.json');
err.requestPath = originalPath;
// TODO(BridgeAR): Add the requireStack as well.
err.requireStack = new Error().stack;
Copy link
Member

@RaisinTen RaisinTen Dec 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @BridgeAR's comment was referring to the requireStack that's generated in

node/lib/internal/modules/cjs/loader.js

Lines 1027 to 1032 in 7811d2d

const requireStack = [];
for (let cursor = parent;
cursor;
cursor = moduleParentCache.get(cursor)) {
ArrayPrototypePush(requireStack, cursor.filename || cursor.id);
}
. It's different from what new Error().stack produces.

@anonrig anonrig requested a review from RaisinTen Dec 5, 2022
JakobJingleheimer
JakobJingleheimer reviewed Jan 2, 2023
View changes
Copy link
Contributor

@JakobJingleheimer JakobJingleheimer left a comment •
edited

Thanks for the contribution!

I'm not super familiar with the CJS space of modules (so not officially 👍), but the implementation LGTM and there was an existing code comment suggesting it is needed, so that seems fine.

@GeoffreyBooth
Copy link
Member

GeoffreyBooth commented Jan 2, 2023

Do we add arbitrary properties to Error anywhere else? @aduh95 ?

@aduh95
Copy link
Contributor

aduh95 commented Jan 2, 2023

Do we add arbitrary properties to Error anywhere else? @aduh95 ?

Yes, e.g. every error from a Node.js API has a code property.

aduh95
aduh95 reviewed Jan 2, 2023
View changes
Copy link
Contributor

@aduh95 aduh95 left a comment •
edited

We'd want to add tests before landing this. Also the same TODO is present twice in the file, IMO it would make sense to address them both at once unless one is more complicated than the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakobJingleheimer JakobJingleheimer

@aduh95 aduh95

@RaisinTen RaisinTen

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mertcanaltin @nodejs-github-bot @GeoffreyBooth @aduh95 @JakobJingleheimer @RaisinTen
X Tutup