module: fix error reporting #55561

geeksilva97 · 2024-10-27T02:54:11Z

The error is incorrectly reported because the traceSync call is on the stack frame. The code that computes the message was getting the first frame expecting it to have the information needed. Something like

at Object.<anonymous> (/Users/edysilva/test-node/issue-55350/test.cjs:1:1)

With traceSync it's like

 at TracingChannel.traceSync (node:diagnostics_channel:322:14)
 at Object.<anonymous> (/Users/edysilva/test-node/issue-55350/test.cjs:1:1)

This PR fixes this behavior by skipping cutting frames about the user's frame.

nodejs-github-bot · 2024-10-27T02:54:16Z

Review requested:

@nodejs/loaders

codecov · 2024-10-27T06:53:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.99%. Comparing base (4ee87b8) to head (7a97799).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55561      +/-   ##
==========================================
- Coverage   88.00%   87.99%   -0.01%     
==========================================
  Files         656      656              
  Lines      189000   189002       +2     
  Branches    35995    35991       -4     
==========================================
- Hits       166320   166313       -7     
- Misses      15840    15847       +7     
- Partials     6840     6842       +2

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	`94.31% <100.00%> (+<0.01%)`	⬆️

... and 23 files with indirect coverage changes

mcollina

Thanks for opening a PR! Can you please add a unit test?

geeksilva97 · 2024-10-27T14:31:13Z

Thanks for opening a PR! Can you please add a unit test?

Sure! It's done 🫡

RafaelGSS · 2024-10-31T03:12:35Z

The PR changes this behavior by making the error reporting to get the last frame on the stack since it will be where all calls were triggered from.

I think this isn't what the error reporting should do? Let me investigate

nodejs-github-bot · 2024-10-31T03:14:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63354/

RafaelGSS

See:

function main() {
  func1(func2(func3()))
}

function func1() {
  require('./app.js')
}
function func2() {}
function func3() {}

main()

Correct output:

➜  undefined git:(55350-issue) ✗ node -v
v22.3.0
➜  undefined git:(55350-issue) ✗ node test.cjs
/Users/rafaelgss/repos/os/node2/undefined/test.cjs:6
  require('./app.js')
  ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/rafaelgss/repos/os/node2/undefined/app.js from /Users/rafaelgss/repos/os/node2/undefined/test.cjs not supported.
Instead change the require of app.js in /Users/rafaelgss/repos/os/node2/undefined/test.cjs to a dynamic import() which is available in all CommonJS modules.
    at func1 (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:6:3)
    at main (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:2:3)
    at Object.<anonymous> (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:11:1) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v22.3.0

After #44340:

➜  undefined git:(55350-issue) ✗ node test.cjs
/Users/rafaelgss/repos/os/node2/undefined/test.cjs:315
undefined
             ^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/rafaelgss/repos/os/node2/undefined/app.js from /Users/rafaelgss/repos/os/node2/undefined/test.cjs not supported.
Instead change the require of app.js in /Users/rafaelgss/repos/os/node2/undefined/test.cjs to a dynamic import() which is available in all CommonJS modules.
    at TracingChannel.traceSync (node:diagnostics_channel:315:14)
    at func1 (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:6:3)
    at main (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:2:3)
    at Object.<anonymous> (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:11:1) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v22.4.0

Your PR:

➜  undefined git:(55350-issue) ✗ ../node --no-experimental-require-module test.cjs
/Users/rafaelgss/repos/os/node2/undefined/test.cjs:11
main()
^

Error [ERR_REQUIRE_ESM]: require() of ES Module /Users/rafaelgss/repos/os/node2/undefined/app.js from /Users/rafaelgss/repos/os/node2/undefined/test.cjs not supported.
Instead change the require of app.js in /Users/rafaelgss/repos/os/node2/undefined/test.cjs to a dynamic import() which is available in all CommonJS modules.
    at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at func1 (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:6:3)
    at main (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:2:3)
    at Object.<anonymous> (/Users/rafaelgss/repos/os/node2/undefined/test.cjs:11:1) {
  code: 'ERR_REQUIRE_ESM'
}

Node.js v24.0.0-pre

I apologize; I haven’t had the bandwidth to look into how the fix should be structured yet. However, this change appears to be inaccurate

geeksilva97 · 2024-10-31T04:34:09Z

The PR changes this behavior by making the error reporting to get the last frame on the stack since it will be where all calls were triggered from.

I think this isn't what the error reporting should do? Let me investigate

Wrong assumption from my side. Thanks for pointing that out. I just pushed a fix.

It takes the first frame after TraseSync. I will also get this case you brought into a test.

lib/internal/modules/cjs/loader.js

RafaelGSS · 2024-10-31T13:40:37Z

Wrong assumption from my side. Thanks for pointing that out. I just pushed a fix.

It takes the first frame after TraseSync. I will also get this case you brought into a test.

Can you try to use hideStackFrames instead? So we don't need to change the error stack trace creation. Example: https://github.com/nodejs/node/blob/main/lib/_http_outgoing.js#L667

geeksilva97 · 2024-11-01T16:32:15Z

Wrong assumption from my side. Thanks for pointing that out. I just pushed a fix.
It takes the first frame after TraseSync. I will also get this case you brought into a test.

Can you try to use hideStackFrames instead? So we don't need to change the error stack trace creation. Example: https://github.com/nodejs/node/blob/main/lib/_http_outgoing.js#L667

Would you guide me how I can do that? I tried to add it in a few places:

wrapping traseSync
wrapping `Module._extensions['.js']
wrapping Module._load

None worked. It ended up messing with the stack.

at TracingChannel.traceSync (node:diagnostics_channel:322:14)
    at wrapModuleLoad (node:internal/modules/cjs/loader:220:24)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:170:5)
    at node:internal/main/run_main_module:36:49 {

It adds this wrapModuleLoad and some more frames.

nodejs-github-bot · 2024-11-26T16:00:11Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63714/

nodejs-github-bot · 2024-11-27T13:40:14Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63723/

nodejs-github-bot · 2024-11-28T14:27:26Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63747/

aduh95 · 2024-11-29T20:17:10Z

@RafaelGSS are you still blocking?

geeksilva97 · 2024-11-29T20:27:40Z

@RafaelGSS are you still blocking?

he wanted to check if all tests would pass before. Tests were failing on windows though. A friend of mine helped me out and I realized it was due to backslashes.

I made some changes - and added some debugging stuff - hopefully, the next CI will pass. If not, at least, it will be clear what is missing. I will then remove this commit

lib/internal/modules/cjs/loader.js

nodejs-github-bot · 2024-11-29T23:38:27Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63796/

geeksilva97 · 2024-11-30T04:51:52Z

lib/internal/modules/cjs/loader.js

@@ -1674,6 +1675,7 @@ function getRequireESMError(mod, pkg, content, filename) {
  const usesEsm = containsModuleSyntax(content, filename);
  const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath,
                                  packageJsonPath);
+  ErrorCaptureStackTrace(err, Module.prototype.require);


The whole stack - including internals for reference - is something like

at getRequireESMError (node:internal/modules/cjs/loader:1683:15) at Object..js (node:internal/modules/cjs/loader:1724:17) at Module.load (node:internal/modules/cjs/loader:1310:32) at Function._load (node:internal/modules/cjs/loader:1124:12) at TracingChannel.traceSync (node:diagnostics_channel:322:14) at wrapModuleLoad (node:internal/modules/cjs/loader:229:12) at Module.require (node:internal/modules/cjs/loader:1333:12) at require (node:internal/modules/helpers:136:16) at Object.<anonymous> (/Users/edysilva/projects/contributions/node/test/fixtures/es-modules/package-type-module/require-esm-error-annotation/index.cjs:1:13) { code: 'ERR_REQUIRE_ESM'

We would then need to strip from require and above.

Using the hideStackFrames would work too - wrapping the Module.prototype.require function.

I don't know the possible impacts of doing that though. That's why I used ErrorCaptureStackTrace.

Let me know if hideStackFrames can be used instead.

cc @RafaelGSS

nodejs-github-bot added module Issues and PRs related to the module subsystem. needs-ci PRs that need a full CI run. labels Oct 27, 2024

geeksilva97 force-pushed the 55350-issue branch 2 times, most recently from 35c501d to 07e16a1 Compare October 27, 2024 04:00

geeksilva97 changed the title ~~loader: fix error reporting (wip)~~ module: fix error reporting (wip) Oct 27, 2024

geeksilva97 marked this pull request as ready for review October 27, 2024 05:45

mcollina reviewed Oct 27, 2024

View reviewed changes

geeksilva97 force-pushed the 55350-issue branch from dc12f2a to dc88885 Compare October 27, 2024 14:30

geeksilva97 changed the title ~~module: fix error reporting (wip)~~ module: fix error reporting Oct 27, 2024

geeksilva97 force-pushed the 55350-issue branch 2 times, most recently from ded0ecb to 7dc8b15 Compare October 27, 2024 14:55

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 31, 2024

RafaelGSS requested changes Oct 31, 2024

View reviewed changes

geeksilva97 requested review from RafaelGSS and mcollina October 31, 2024 04:49

geeksilva97 force-pushed the 55350-issue branch 2 times, most recently from 9847e2d to 00dfc8f Compare October 31, 2024 04:51

geeksilva97 commented Oct 31, 2024

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

jasnell approved these changes Nov 5, 2024

View reviewed changes

geeksilva97 force-pushed the 55350-issue branch from 00dfc8f to 5b0e7f6 Compare November 26, 2024 15:52

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 26, 2024

geeksilva97 force-pushed the 55350-issue branch from 8234109 to 6f463de Compare November 26, 2024 20:49

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 27, 2024

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 28, 2024

geeksilva97 force-pushed the 55350-issue branch 3 times, most recently from ce81fff to e0ccecc Compare November 28, 2024 18:17

RafaelGSS added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024

RafaelGSS reviewed Nov 29, 2024

View reviewed changes

lib/internal/modules/cjs/loader.js Outdated Show resolved Hide resolved

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 29, 2024

geeksilva97 force-pushed the 55350-issue branch 2 times, most recently from 7eda507 to 96f5831 Compare November 30, 2024 04:50

geeksilva97 commented Nov 30, 2024

View reviewed changes

geeksilva97 force-pushed the 55350-issue branch from 96f5831 to 9c64a98 Compare November 30, 2024 04:55

module: fix error reporting when commonjs requires an ES module

9cda6cf

geeksilva97 force-pushed the 55350-issue branch from 9c64a98 to 9cda6cf Compare November 30, 2024 04:56

test: improve matching

7a97799

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

module: fix error reporting #55561

module: fix error reporting #55561

geeksilva97 commented Oct 27, 2024 •

edited

Loading

nodejs-github-bot commented Oct 27, 2024

codecov bot commented Oct 27, 2024 •

edited

Loading

mcollina left a comment

geeksilva97 commented Oct 27, 2024

RafaelGSS commented Oct 31, 2024

nodejs-github-bot commented Oct 31, 2024

RafaelGSS left a comment

geeksilva97 commented Oct 31, 2024

RafaelGSS commented Oct 31, 2024

geeksilva97 commented Nov 1, 2024 •

edited

Loading

nodejs-github-bot commented Nov 26, 2024

nodejs-github-bot commented Nov 27, 2024

nodejs-github-bot commented Nov 28, 2024

aduh95 commented Nov 29, 2024

geeksilva97 commented Nov 29, 2024

nodejs-github-bot commented Nov 29, 2024

geeksilva97 Nov 30, 2024 •

edited

Loading

geeksilva97 Nov 30, 2024 •

edited

Loading

module: fix error reporting #55561

Are you sure you want to change the base?

module: fix error reporting #55561

Conversation

geeksilva97 commented Oct 27, 2024 • edited Loading

nodejs-github-bot commented Oct 27, 2024

codecov bot commented Oct 27, 2024 • edited Loading

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

geeksilva97 commented Oct 27, 2024

RafaelGSS commented Oct 31, 2024

nodejs-github-bot commented Oct 31, 2024

RafaelGSS left a comment

Choose a reason for hiding this comment

geeksilva97 commented Oct 31, 2024

RafaelGSS commented Oct 31, 2024

geeksilva97 commented Nov 1, 2024 • edited Loading

nodejs-github-bot commented Nov 26, 2024

nodejs-github-bot commented Nov 27, 2024

nodejs-github-bot commented Nov 28, 2024

aduh95 commented Nov 29, 2024

geeksilva97 commented Nov 29, 2024

nodejs-github-bot commented Nov 29, 2024

geeksilva97 Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

geeksilva97 Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

geeksilva97 commented Oct 27, 2024 •

edited

Loading

codecov bot commented Oct 27, 2024 •

edited

Loading

geeksilva97 commented Nov 1, 2024 •

edited

Loading

geeksilva97 Nov 30, 2024 •

edited

Loading

geeksilva97 Nov 30, 2024 •

edited

Loading