Added the requireStack to loader.js #45734

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

@@ -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;


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 takes the error object and a function as arguments.
+      // The function is used as a marker for the point in the stack trace where the capture should begin.
+      // In this case, we are using the anonymous function that follows as the marker.
+      Error.captureStackTrace(err, () => {});


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

@@ -0,0 +1,56 @@
+import { tryPackage }  from '../../lib/internal/modules/cjs/loader'


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

@@ -0,0 +1,56 @@
+import { tryPackage }  from '../../lib/internal/modules/cjs/loader'


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

+const assert = require('assert');
+
+describe('tryPackage', () => {
+  it('should return the file path when the package is found and main is specified', () => {


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

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

lib/internal/modules/cjs/loader.js

+      // The function is called for each stack frame in the stack trace, and it should return a string representation of the stack frame.
+      // In the following example, we use the `util.inspect()` function to get a string representation of each stack frame.
+      // We then check if the file name of the stack frame is a core module or an internal Node.js module by checking if it starts with `'internal/'` or `'module'`.
+      // If it is not a core module or an internal Node.js module, we add the file name to the `requireStack` array:


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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added the requireStack to loader.js #45734

Added the requireStack to loader.js #45734

mertcanaltin commented Dec 4, 2022

nodejs-github-bot commented Dec 4, 2022

RaisinTen Dec 4, 2022

JakobJingleheimer left a comment •

edited

Loading

GeoffreyBooth commented Jan 2, 2023

aduh95 commented Jan 2, 2023

aduh95 left a comment •

edited

Loading

aduh95 left a comment

RaisinTen commented Jan 9, 2023

GeoffreyBooth Jan 9, 2023

RaisinTen Jan 11, 2023

GeoffreyBooth Jan 11, 2023

GeoffreyBooth Jan 11, 2023

jasnell Feb 7, 2023

mertcanaltin commented Feb 9, 2023

JakobJingleheimer commented Feb 9, 2023

MoLow commented Feb 9, 2023

mertcanaltin commented Feb 9, 2023

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

		@@ -0,0 +1,56 @@
		import { tryPackage } from '../../lib/internal/modules/cjs/loader'

Added the requireStack to loader.js #45734

Added the requireStack to loader.js #45734

Conversation

mertcanaltin commented Dec 4, 2022

nodejs-github-bot commented Dec 4, 2022

RaisinTen Dec 4, 2022

Choose a reason for hiding this comment

JakobJingleheimer left a comment • edited Loading

Choose a reason for hiding this comment

GeoffreyBooth commented Jan 2, 2023

aduh95 commented Jan 2, 2023

aduh95 left a comment • edited Loading

Choose a reason for hiding this comment

aduh95 left a comment

Choose a reason for hiding this comment

RaisinTen commented Jan 9, 2023

GeoffreyBooth Jan 9, 2023

Choose a reason for hiding this comment

RaisinTen Jan 11, 2023

Choose a reason for hiding this comment

GeoffreyBooth Jan 11, 2023

Choose a reason for hiding this comment

GeoffreyBooth Jan 11, 2023

Choose a reason for hiding this comment

jasnell Feb 7, 2023

Choose a reason for hiding this comment

mertcanaltin commented Feb 9, 2023

JakobJingleheimer commented Feb 9, 2023

MoLow commented Feb 9, 2023

mertcanaltin commented Feb 9, 2023

JakobJingleheimer left a comment •

edited

Loading

aduh95 left a comment •

edited

Loading