module: improve error message for top-level await in CommonJS #55874
Conversation
nodejs-github-bot
added
c++
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #55874 +/- ##
==========================================
- Coverage 88.42% 88.41% -0.01%
==========================================
Files 654 654
Lines 187852 187859 +7
Branches 36134 36136 +2
==========================================
- Hits 166102 166097 -5
- Misses 14989 15006 +17
+ Partials 6761 6756 -5
|
| @@ -1611,7 +1611,9 @@ static std::vector<std::string_view> throws_only_in_cjs_error_messages = { | |||
| "Identifier '__filename' has already been declared", | |||
| "Identifier '__dirname' has already been declared", | |||
| "await is only valid in async functions and " | |||
| "the top level bodies of modules"}; | |||
| "the top level bodies of modules", | |||
| "Identifier 'require' has already been declared", | |||
There was a problem hiding this comment.
"Identifier 'require' has already been declared" is already in this list (look five lines up, on line 1610).
| for (const auto& error_message : throws_only_in_cjs_error_messages) { | ||
| if (message_view.find(error_message) != std::string_view::npos) { | ||
| isolate->ThrowException(v8::Exception::SyntaxError( | ||
| String::NewFromUtf8( | ||
| isolate, | ||
| "Top-level await is not supported in CommonJS. " | ||
| "Consider using ESM or wrap await in an async function.") | ||
| .ToLocalChecked())); |
There was a problem hiding this comment.
It seems like this message is being printed for any of the “throws only in CJS” error messages? I would think that we would want to print this only when the user wrote a top-level await in a file that didn’t parse successfully as ESM.
Also in general our messages don’t use “ESM” as that’s not a widely known term. I think we usually write “module syntax” or “import/export syntax”.
There was a problem hiding this comment.
thank you very much for the improvement suggestions, I wonder if you could comment on the broken test/es-module/test-esm-detect-ambiguous.mjs tests, I am trying to improve them
in addition, I wonder if it would be better to include my newly added test/parallel/test-commonjs-top-level-await.js test in test-esm-detect-ambiguous.mjs
There was a problem hiding this comment.
What test failure do you see?
in addition, I wonder if it would be better to include my newly added test/parallel/test-commonjs-top-level-await.js test in test-esm-detect-ambiguous.mjs
Yes, my preference would be to keep all the syntax detection tests together. If you could follow the pattern of the other related tests that would be great.
There was a problem hiding this comment.
There was a problem hiding this comment.
I saw this
nodejs/node/actions/runs/11863231944/job/33064268346?pr=55874
I don't see the failure in this output. When you run the tests locally, what fails?
There was a problem hiding this comment.
when I run the tests locally:
./node test/es-module/test-esm-detect-ambiguous.mjs
I see that 7 tests are exploding, I think the code we do error handling is crushing the expectations here
failing tests:
✖ does not warn when there are no package.json
✖ permits declaration of CommonJS module variables
✖ throws on undefined require when top-level await triggers ESM parsing
✖ still throws on await in an ordinary sync function
✖ permits top-level await above import/export syntax
✖ reports unfinished top-level await
✖ permits top-level await
I'm trying to fix this place.
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