FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

module: Improves Top-Level await error in cjs by bdougie · Pull Request #35196 · nodejs/node · GitHub

/ node Public

module: Improves Top-Level await error in cjs - #35196

Closed
bdougie wants to merge 1 commit into
nodejs:masterfrom
bdougie:top-level-await-error
ConversationCommits (1)ChecksFiles changed
Closed

module: Improves Top-Level await error in cjs#35196
bdougie wants to merge 1 commit into
nodejs:masterfrom
bdougie:top-level-await-error

Conversation

bdougie commented Sep 14, 2020

Copy link
Copy Markdown
Contributor

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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

module: Improves Top-Level await error in cjs
59ba7ad 
Top-Level await is support in esm and not in cjs. The default error
message is confusing.

nodejs-github-bot commented Sep 14, 2020

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/modules

nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Sep 14, 2020
bdougie commented Sep 14, 2020
View reviewed changes
Comment thread .eslintignore
lib/punycode.js
test/addons/??_*
test/fixtures
test/message/cjs_top_await_error.js

bdougie Sep 14, 2020

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This is here because eslint doesn't play well with top-level await yet.

MylesBorins approved these changes Sep 14, 2020
View reviewed changes

MylesBorins left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

MylesBorins commented Sep 14, 2020

Copy link
Copy Markdown
Contributor

Thanks @bdougie! CI is locked down right now for a security release, but we'll get the full suite kicked off once we can.

devsnek commented Sep 14, 2020

Copy link
Copy Markdown
Member

this almost seems like it would be better to do upstream.

bdougie commented Sep 14, 2020

Copy link
Copy Markdown
Contributor Author

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?

devsnek commented Sep 14, 2020

Copy link
Copy Markdown
Member

I mean changing this in V8 itself, instead of modifying the message in node.

mscdex commented Sep 15, 2020

Copy link
Copy Markdown
Contributor

I agree changing this in V8 would be a better solution than having to maintain a error message string check like this.

MylesBorins commented Sep 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

MylesBorins commented Sep 15, 2020

Copy link
Copy Markdown
Contributor

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

devsnek reviewed Sep 15, 2020
View reviewed changes
Comment thread lib/internal/modules/cjs/loader.js
} catch (err) {
if (process.mainModule === cjsModuleInstance)
enrichCJSError(err);
if (err.message.includes('await is only valid in async function')) {

devsnek Sep 15, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

what happens if you plug in this code:

function x() {
  await 1;
}

MylesBorins Sep 15, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality 
/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:47

Yeah, this is not good. Do you think there is a way to distinguish this without having to parse the entire file?

targos commented Sep 15, 2020

Copy link
Copy Markdown
Member

Top-level await is not specific to the node runtime, is it?
I agree that this would be better as an upstream change and maybe also easier to implement (to distinguish an attempt to use TLA vs await in a regular function)

devsnek commented Sep 15, 2020

Copy link
Copy Markdown
Member

https://chromium-review.googlesource.com/c/v8/v8/+/2411687

MylesBorins commented Sep 15, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

thanks for opening that @devsnek I can work with @bdougie on backporting if it lands 🎉

devsnek commented Sep 23, 2020

Copy link
Copy Markdown
Member

landed above as 4263f8a5e8e04a766aeb7cde0081da3ac6c12a9e in v8

bdougie mentioned this pull request Oct 14, 2020
Closed
4 tasks

MylesBorins commented Oct 14, 2020

Copy link
Copy Markdown
Contributor

Closing in lieu of #35650

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
MylesBorins MylesBorins approved these changes
devsnek devsnek left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

tools Issues and PRs related to the tools directory.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL