Added the requireStack to loader.js by mertcanaltin · Pull Request #45734 · nodejs/node

mertcanaltin · 2022-12-04T11:35:19Z

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

I also added the require stack

nodejs-github-bot · 2022-12-04T11:35:23Z

Review requested:

@nodejs/modules

RaisinTen · 2022-12-04T11:59:34Z

lib/internal/modules/cjs/loader.js

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.

JakobJingleheimer

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 · 2023-01-02T15:59:57Z

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

aduh95 · 2023-01-02T19:00:54Z

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

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.

aduh95

What is the use case that's would be fixed by it? I think we should start with writing some tests for the feature so we set the expectations for what we want the feature to look like, and then proceed with the implementation. Adding an empty array to an error object doesn't make much sense to me if I can't see in what context it can be used, having tests for that would help greatly.

RaisinTen · 2023-01-09T05:20:39Z

The use case for this property has been discussed in #25690. As suggested in #45734 (comment), this array should be populated with paths of non-internal modules that are present in the stack trace.

GeoffreyBooth · 2023-01-09T17:15:55Z

lib/internal/modules/cjs/loader.js

This function is a hook for users to register their own stack trace customizations. I’m not sure we should be using it internally.

RaisinTen · 2023-01-11T06:12:51Z

test/parallel/test-loader.js

This can be tested using the public APIs alone, so I would suggest you to revert the tryPackage exporting change from lib/internal/modules/cjs/loader.js and instead create a temporary file structure of this sort dynamically while running the test:

❯ tree -a . ├── index.js └── node_modules ├── a │ ├── a.js │ └── package.json └── b └── package.json 3 directories, 4 files ❯ cat index.js require('a'); ❯ cat node_modules/a/a.js require('b'); ❯ cat node_modules/a/package.json { "main": "a.js" } ❯ cat node_modules/b/package.json { "main": "b.js" }

and assert on the requireStack property of the error thrown by the index.js file.

Also, note that this is a CJS file, so the way you used import here won't work.

GeoffreyBooth · 2023-01-11T06:38:31Z

test/parallel/test-loader.js

test-loader.js is a much too generic name. test-require-package-errors.js perhaps?

Or even better, find where we currently test the errors potentially thrown by require and add your new cases there.

GeoffreyBooth · 2023-01-11T06:39:05Z

test/parallel/test-loader.js

Surely there’s an existing test that require of a package works?

jasnell · 2023-02-07T00:23:58Z

lib/internal/modules/cjs/loader.js

Nit: line lengths here should be <= 100 chars.

mertcanaltin · 2023-02-09T12:45:38Z

uh 🆘

JakobJingleheimer · 2023-02-09T12:48:26Z

I can't re-open it 🤨

MoLow · 2023-02-09T12:55:09Z

I guess it cannot be reopened since it has no commits

mertcanaltin · 2023-02-09T13:33:49Z

I'm so sorry I accidentally blew my code here :(
i created a pr : #46586

nodejs-github-bot added the needs-ci PRs that need a full CI run. label Dec 4, 2022

RaisinTen reviewed Dec 4, 2022

View reviewed changes

anonrig requested a review from RaisinTen December 5, 2022 22:45

JakobJingleheimer reviewed Jan 2, 2023

View reviewed changes

aduh95 reviewed Jan 2, 2023

View reviewed changes

aduh95 reviewed Jan 8, 2023

View reviewed changes

GeoffreyBooth reviewed Jan 9, 2023

View reviewed changes

RaisinTen reviewed Jan 11, 2023

View reviewed changes

GeoffreyBooth reviewed Jan 11, 2023

View reviewed changes

mertcanaltin requested a review from RaisinTen January 21, 2023 18:00

jasnell reviewed Feb 7, 2023

View reviewed changes

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

Copy link

Member

jasnell Feb 7, 2023

Choose a reason for hiding this comment

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

Nit: line lengths here should be <= 100 chars.

mertcanaltin reacted with heart emoji

mertcanaltin force-pushed the main branch from 57f4e00 to 7d7413c Compare February 9, 2023 12:32

mertcanaltin closed this Feb 9, 2023

mertcanaltin force-pushed the main branch from 7d7413c to 5092346 Compare February 9, 2023 12:42

	const requireStack = [];
	for (let cursor = parent;
	cursor;
	cursor = moduleParentCache.get(cursor)) {
	ArrayPrototypePush(requireStack, cursor.filename \|\| cursor.id);
	}

Uh oh!

Conversation

mertcanaltin commented Dec 4, 2022

Uh oh!

nodejs-github-bot commented Dec 4, 2022

Uh oh!

RaisinTen Dec 4, 2022

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth commented Jan 2, 2023

Uh oh!

aduh95 commented Jan 2, 2023

Uh oh!

aduh95 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduh95 left a comment

Choose a reason for hiding this comment

Uh oh!

RaisinTen commented Jan 9, 2023

Uh oh!

GeoffreyBooth Jan 9, 2023

Choose a reason for hiding this comment

Uh oh!

RaisinTen Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Jan 11, 2023

Choose a reason for hiding this comment

Uh oh!

jasnell Feb 7, 2023

Choose a reason for hiding this comment

Uh oh!

mertcanaltin commented Feb 9, 2023

Uh oh!

JakobJingleheimer commented Feb 9, 2023

Uh oh!

MoLow commented Feb 9, 2023

Uh oh!

mertcanaltin commented Feb 9, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Comments

JakobJingleheimer left a comment •

edited

Loading

aduh95 left a comment •

edited

Loading