module: Improves Top-Level await error in cjs #35196
Conversation
Top-Level await is support in esm and not in cjs. The default error message is confusing.
|
Review requested:
|
| @@ -3,6 +3,7 @@ lib/internal/v8_prof_polyfill.js | |||
| lib/punycode.js | |||
| test/addons/??_* | |||
| test/fixtures | |||
| test/message/cjs_top_await_error.js | |||
There was a problem hiding this comment.
This is here because eslint doesn't play well with top-level await yet.
|
Thanks @bdougie! CI is locked down right now for a security release, but we'll get the full suite kicked off once we can. |
|
this almost seems like it would be better to do upstream. |
@devsnek, can you expand on what you mean by upstream? Is there a better place for the message? |
|
I mean changing this in V8 itself, instead of modifying the message in node. |
|
I agree changing this in V8 would be a better solution than having to maintain a error message string check like this. |
|
We already have done this once, catching and editing the message in the repl. Improving this error in V8 is absolutely something we can do but it will likely have to wait until V8 itself unflags Top-Level Await. In the mean time this seems like a significant improvement to the status quo with very little overhead prior art: https://github.com/nodejs/node/blob/master/lib/repl.js#L582-L591 |
|
It also worth mentioning that the replaced error message is specifically referencing common.js, a unique goal from the script goal. V8 would not land a message specific to the node runtime |
| @@ -1203,6 +1203,11 @@ function wrapSafe(filename, content, cjsModuleInstance) { | |||
| } catch (err) { | |||
| if (process.mainModule === cjsModuleInstance) | |||
| enrichCJSError(err); | |||
| if (err.message.includes('await is only valid in async function')) { | |||
There was a problem hiding this comment.
what happens if you plug in this code:
function x() {
await 1;
}There was a problem hiding this comment.
/Users/mylesborins/code/node/main/lol.js:2
await 1;
^^^^^
SyntaxError: Top-Level await is only supported in ESM.
at wrapSafe (internal/modules/cjs/loader.js:1005:16)
at Module._compile (internal/modules/cjs/loader.js:1058:27)
at Object.Module._extensions..js (internal/modules/cjs/loader.js:1115:10)
at Module.load (internal/modules/cjs/loader.js:954:32)
at Function.Module._load (internal/modules/cjs/loader.js:795:14)
at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:72:12)
at internal/main/run_main_module.js:17:47Yeah, this is not good. Do you think there is a way to distinguish this without having to parse the entire file?
|
Top-level await is not specific to the node runtime, is it? |
|
landed above as 4263f8a5e8e04a766aeb7cde0081da3ac6c12a9e in v8 |
|
Closing in lieu of #35650 |
Top-Level await is supported in ESM and not in cjs. The default error
the message is confusing. I provided a clearer one and opened this PR with a lot of help from @MylesBorins.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes