module: improve error message for top-level await in CommonJS #55874
Conversation
Codecov ReportAttention: Patch coverage is
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
🚀 New features to boost your workflow:
|
76f87e4 to
c9531ef
Compare
There was a problem hiding this comment.
Thanks for taking another pass at this. I mention this in the particular notes, but in general:
-
Please don't change lines based on personal preference or readability. We value the ability for
git blameto 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. -
Very few, if any, tests should change as a result of your PR: only tests specifically related to top-level
awaitin 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-levelawait. If there aren't currently tests for top-levelawaitin 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 |
|
Greetings, when I did as you suggested to cover only the top level error, I saw that all tests passed except 1 test |
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? |
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. |
|
@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 |
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 #49120 but that got closed. |
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'
} |
|
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 |
|
hello, I would be very grateful if you could look here when you are available, thank you very much @GeoffreyBooth |
| isCommonJSGlobalLikeNotDefinedError(e.message)) { | ||
|
|
||
| if (hasTopLevelAwait) { | ||
| e.message = `ERR_AMBIGUOUS_MODULE_SYNTAX: This file cannot be parsed as either CommonJS or ES Module. CommonJS error: await is only valid in async functions. ES Module error: require is not defined in ES module scope. If you meant to use CommonJS, wrap top-level await in async function. If you meant to use ESM, do not use require().`; |
There was a problem hiding this comment.
Okay, I'm a little confused here -- the code mostly looks good, but maybe we can spell out which condition exactly is supposed to trigger this error, and be specific about the error message? Like, it's odd to describe multiple different errors here, although I kind of see where you're coming from
There was a problem hiding this comment.
Hi @addaleax,
You're absolutely right - the message is trying to do too much.
The specific condition is: file has both require() and top-level await, creating an ambiguous parsing situation.
How about we simplify the error message to focus on the immediate issue:
ERR_AMBIGUOUS_MODULE_SYNTAX: Cannot use 'require()' and top-level 'await' in the same file.
Choose either CommonJS (remove await, use async functions) or ES Module (remove require, use import).There was a problem hiding this comment.
Technically it can be parsed as ESM, just not successfully evaluated as ESM because require is undefined. It can't be successfully parsed as CommonJS because of the top-level await. So I'm not sure if "ambiguous" is the right word; it's more that it's so unclear what the user intended that we can't reasonably make a guess.
There was a problem hiding this comment.
Technically it can be parsed as ESM, just not successfully evaluated as ESM because
requireis undefined. It can't be successfully parsed as CommonJS because of the top-level await. So I'm not sure if "ambiguous" is the right word; it's more that it's so unclear what the user intended that we can't reasonably make a guess.
Yes, I agree with what you said, I think a message like this would be more appropriate.
Cannot determine intended module format. File contains both 'require()' and top-level 'await'. For CommonJS: wrap await in async function. For ES Module: replace require() with import.There was a problem hiding this comment.
I assume when you write For CommonJS: what you mean is, If you intended this file to be CommonJS, and likewise for For ES Module:. So maybe:
Cannot determine intended module format because both
require()and top-levelawaitare present. If the code is intended to be CommonJS, wrapawaitin an async function. If the code is intended to be an ES module, replacerequire()withimport.
|
Landed in 6ce6fdb |
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