module: improve error message for top-level await in CommonJS

[mertcanaltin]

Added a specific error message for using top-level await in CommonJS modules. The error now suggests using ESM or wrapping await in an async function for clarity.

Fixes: #55776

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.08%. Comparing base (968e2f4) to head (ad40103). Report is 153 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	69.23%	2 Missing and 2 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #55874      +/-   ##
==========================================
- Coverage   90.21%   90.08%   -0.13%     
==========================================
  Files         635      640       +5     
  Lines      187580   188384     +804     
  Branches    36853    36937      +84     
==========================================
+ Hits       169231   169712     +481     
- Misses      11108    11396     +288     
- Partials     7241     7276      +35     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/module_job.js	97.48% <100.00%> (+0.26%)	:arrow_up:
src/module_wrap.h	0.00% <ø> (ø)
src/module_wrap.cc	72.59% <69.23%> (+0.15%)	:arrow_up:

... and 124 files with indirect coverage changes

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[mertcanaltin]

Thanks for taking another pass at this. I mention this in the particular notes, but in general:

1. Please don't change lines based on personal preference or readability. We value the ability for git blame to point directly to the commit/PR that added or meaningfully changed a particular line, and that ability is diluted when unrelated PRs rewrite lines of code.
2. Very few, if any, tests should change as a result of your PR: only tests specifically related to top-level await in CommonJS. Any other tests that fail as a result of your changes mean that there's a bug in the implementation that needs to be addressed. My guess is that the bug is that the new code isn't checking that the error message being thrown is specifically about top-level await. If there aren't currently tests for top-level await in CommonJS and your new error message, one or more should be added.

This is close; another pass and I think you may get it there. Thanks for your effort!

thank you very much for the review, I will be sending a new commit following what you said

[mertcanaltin]

Greetings, when I did as you suggested to cover only the top level error, I saw that all tests passed except 1 test

[mertcanaltin]

Thanks, this is much better. What’s the one test that is failing?

I saw wrong, all the tests passed my local. 🙏

[GeoffreyBooth]

I saw wrong, all the tests passed my local. 🙏

There’s a test failing in here, in the relevant file: https://github.com/nodejs/node/actions/runs/12133213207/job/33828380981?pr=55874

@cjihrig why isn’t the CI output showing me which test is failing within that file?

[cjihrig]

why isn’t the CI output showing me which test is failing within that file?

No clue. But the output is incomplete. There is no summary at the end either. It looks like maybe the Python runner killed the Node process or otherwise truncated its output.

[cjihrig]

@GeoffreyBooth just FYI - IIRC, you asked me a similar question in Slack last year. There was output truncated only in GitHub Actions. Then you asked me to find a test run where truncation was happening without the use of node:test, and I did. I believe @MoLow was a part of that thread as well.

[GeoffreyBooth]

IIRC, you asked me a similar question in Slack last year.

Yes, last August: https://openjs-foundation.slack.com/archives/C019Y2T6STH/p1691860008875109. I’m lucky if I can remember last month, much less last year. Sorry to repeat. I also opened https://github.com/nodejs/node/issues/49120 but that got closed.

[mertcanaltin]

I saw wrong, all the tests passed my local. 🙏

There’s a test failing in here, in the relevant file: https://github.com/nodejs/node/actions/runs/12133213207/job/33828380981?pr=55874

@cjihrig why isn’t the CI output showing me which test is failing within that file?

yes, ı view my local enviorement this test is fail

test at test/es-module/test-esm-detect-ambiguous.mjs:367:5
✖ does not warn when there are no package.json (63.625667ms)
  AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
  + actual - expected
  
  + "(node:18411) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/mert/Desktop/openSource/node/test/fixtures/es-modules/loose.js is not specified and it doesn't parse as CommonJS.\n" +
  +   'Reparsing as ES module because module syntax was detected. This incurs a performance overhead.\n' +
  +   'To eliminate this warning, add "type": "module" to /Users/mert/package.json.\n' +
  +   '(Use `node --trace-warnings ...` to show where the warning was created)\n'
  - ''
  
      at TestContext.<anonymous> (file:///Users/mert/Desktop/openSource/node/test/es-module/test-esm-detect-ambiguous.mjs:372:7)
      at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
      at async Test.run (node:internal/test_runner/test:932:9)
      at async Promise.all (index 3)
      at async Suite.run (node:internal/test_runner/test:1310:7)
      at async Promise.all (index 6)
      at async Suite.run (node:internal/test_runner/test:1310:7)
      at async startSubtestAfterBootstrap (node:internal/test_runner/harness:297:3) {
    generatedMessage: true,
    code: 'ERR_ASSERTION',
    actual: '(node:18411) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///Users/mert/Desktop/openSource/node/test/fixtures/es-modules/loose.js is not specified and it doesn\'t parse as CommonJS.\nReparsing as ES module because module syntax was detected. This incurs a performance overhead.\nTo eliminate this warning, add "type": "module" to /Users/mert/package.json.\n(Use `node --trace-warnings ...` to show where the warning was created)\n',
    expected: '',
    operator: 'strictEqual'
  }

[mertcanaltin]

greetings I tried to make the error message more descriptive, in addition I tried to throw it when there is a file that cannot be successfully parsed as ESM

[mertcanaltin]

hello, I would be very grateful if you could look here when you are available, thank you very much @GeoffreyBooth

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[mertcanaltin]

@mertcanaltin Thank you for your persistence. I think the approach of “a test is failing, so I’ll update the test” isn’t serving us well here; most of these updated tests shouldn’t need updating to achieve the goals of this PR, and the fact that they’re failing is an indicator that there are bugs in the implementation.

many thanks for your answers I will improve this place

[joyeecheung]

I feel for the specific case mentioned in https://github.com/nodejs/node/issues/55776, merely amending the error message is not good enough. It would've been clearer to either completely revert to the CJS exception (i.e. even the source line should be where the top-level await is at, not where require is at), or show both the exception that would be thrown if it's parsed as CJS, and the exception that would be thrown if it's parsed as ESM, to make it clear to the users that it cannot be parsed as neither, and print solution for each one as a warning (if they meant to use CJS, wrap the await in async function; if they meant to use ESM, do not use require).

[mertcanaltin]

I feel for the specific case mentioned in #55776, merely amending the error message is not good enough. It would've been clearer to either completely revert to the CJS exception (i.e. even the source line should be where the top-level await is at, not where require is at), or show both the exception that would be thrown if it's parsed as CJS, and the exception that would be thrown if it's parsed as ESM, to make it clear to the users that it cannot be parsed as neither, and print solution for each one as a warning (if they meant to use CJS, wrap the await in async function; if they meant to use ESM, do not use require).

Hello, now I tried to highlight both errors that can occur as you said

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in 6ce6fdbf219a72ecdbe5e65d0259f1072304b1d9