GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — module: make synthetic module evaluation steps return a Promise to support top level await by dandclark · Pull Request #37300 · nodejs/node

Conversation

@dandclark

@dandclark dandclark commented Feb 9, 2021

Copy link
Copy Markdown
Contributor

Top level await expects that all module script evaluation returns a
Promise. As such, update
ModuleWrap::SyntheticModuleEvaluationStepsCallback to return a
resolved Promise now that V8 has enabled top-level await by default.

Unfortunately I don't have a spec reference that I can point to other
than https://tc39.es/proposal-top-level-await/ because the built-in
modules proposal isn't yet updated for top level await.

The corresponding change for Blink is
https://chromium-review.googlesource.com/c/chromium/src/+/2568823.

Resolves #37299

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@dandclark
module: make synthetic module evaluation steps return a Promise to su…
bbfb155 
…pport top level await

Top level await expects that all module script evaluation returns a
Promise. As such, update
ModuleWrap::SyntheticModuleEvaluationStepsCallback to return a
resolved Promise now that V8 has enabled top-level await by default.

Unfortunately I don't have a spec reference that I can point to here
because the Built-in modules proposal isn't yet updated for
top level await.

The corresponding change for Blink is
https://chromium-review.googlesource.com/c/chromium/src/+/2568823.

This will allow a workaround for Node in this V8 bugfix to be removed:
https://chromium-review.googlesource.com/c/v8/v8/+/2673794.

Fixes: nodejs#37299
@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 9, 2021
@aduh95
aduh95 requested a review from devsnek February 9, 2021 21:42
devsnek
devsnek approved these changes Feb 9, 2021
View reviewed changes
@devsnek devsnek added esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. labels Feb 9, 2021
@nodejs-github-bot

nodejs-github-bot commented Feb 9, 2021

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/36049/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@aduh95 aduh95 added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Feb 9, 2021
@aduh95

aduh95 commented Feb 10, 2021

Copy link
Copy Markdown
Contributor

Does this has any effect on the public vm API?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@devsnek

devsnek commented Feb 10, 2021

Copy link
Copy Markdown
Member

nope
aduh95 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos
targos approved these changes Feb 12, 2021
View reviewed changes
@devsnek

devsnek commented Feb 12, 2021

Copy link
Copy Markdown
Member

landed in 521c08d
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@devsnek devsnek closed this Feb 12, 2021
devsnek pushed a commit that referenced this pull request Feb 12, 2021
@dandclark @devsnek
module: make synthetic module evaluation steps return a Promise to su…
521c08d 
…pport top level await

Top level await expects that all module script evaluation returns a
Promise. As such, update
ModuleWrap::SyntheticModuleEvaluationStepsCallback to return a
resolved Promise now that V8 has enabled top-level await by default.

Unfortunately I don't have a spec reference that I can point to here
because the Built-in modules proposal isn't yet updated for
top level await.

The corresponding change for Blink is
https://chromium-review.googlesource.com/c/chromium/src/+/2568823.

This will allow a workaround for Node in this V8 bugfix to be removed:
https://chromium-review.googlesource.com/c/v8/v8/+/2673794.

Fixes: #37299

PR-URL: #37300
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
danielleadams pushed a commit that referenced this pull request Feb 16, 2021
@dandclark @danielleadams
module: make synthetic module evaluation steps return a Promise to su…
d96a97a 
…pport top level await

Top level await expects that all module script evaluation returns a
Promise. As such, update
ModuleWrap::SyntheticModuleEvaluationStepsCallback to return a
resolved Promise now that V8 has enabled top-level await by default.

Unfortunately I don't have a spec reference that I can point to here
because the Built-in modules proposal isn't yet updated for
top level await.

The corresponding change for Blink is
https://chromium-review.googlesource.com/c/chromium/src/+/2568823.

This will allow a workaround for Node in this V8 bugfix to be removed:
https://chromium-review.googlesource.com/c/v8/v8/+/2673794.

Fixes: #37299

PR-URL: #37300
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
This was referenced Feb 16, 2021
v15.9.0 proposal #37405
Closed
v15.9.0 proposal #37406
Merged
@targos targos mentioned this pull request Feb 18, 2021
Node.js 15.9 regression with Jest ESM #37426
Closed
@ai ai mentioned this pull request Feb 18, 2021
ESM regression on Node.js 15.9 jestjs/jest#11093
Closed
@ai

ai commented Feb 23, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

@devsnek Node.js 15.10 still has this issue. Did your fix was released in 15.10?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
targos pushed a commit that referenced this pull request May 1, 2021
@dandclark @targos
module: make synthetic module evaluation steps return a Promise to su…
4cdd9b6 
…pport top level await

Top level await expects that all module script evaluation returns a
Promise. As such, update
ModuleWrap::SyntheticModuleEvaluationStepsCallback to return a
resolved Promise now that V8 has enabled top-level await by default.

Unfortunately I don't have a spec reference that I can point to here
because the Built-in modules proposal isn't yet updated for
top level await.

The corresponding change for Blink is
https://chromium-review.googlesource.com/c/chromium/src/+/2568823.

This will allow a workaround for Node in this V8 bugfix to be removed:
https://chromium-review.googlesource.com/c/v8/v8/+/2673794.

Fixes: #37299

PR-URL: #37300
Reviewed-By: Gus Caplan <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
@danielleadams danielleadams mentioned this pull request May 3, 2021
v14.17.0 proposal #38507
Merged
@RafaelGSS RafaelGSS mentioned this pull request Nov 1, 2021
Flamegraphs is not working properly in v16+ clinicjs/node-clinic#293
Closed
@RafaelGSS RafaelGSS mentioned this pull request Dec 26, 2021
fix: increase highWaterMark value to fix file descriptor issue on v16+ clinicjs/node-clinic-bubbleprof#389
Merged
@wedataintelligence wedataintelligence mentioned this pull request Jan 12, 2023
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 wedataintelligence/node#72
Open
This was referenced Jan 12, 2023
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.22 to 5.25.1 kissofire/node#93
Open
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 molllyn1/node#74
Open
@137717unity 137717unity mentioned this pull request Jan 13, 2023
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 137717unity/node#91
Open
This was referenced Jan 13, 2023
[Snyk] Security upgrade gatsby-transformer-remark from 2.8.27 to 5.25.1 baby636/node#87
Open
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 ron-a20/node#91
Open
This was referenced Jan 13, 2023
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 Rednickle/node-1#69
Open
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 aliceUnhinged613/node#88
Open
[Snyk] Security upgrade gatsby-transformer-remark from 2.6.45 to 5.25.1 turkdevops/node#586
Open
This was referenced Apr 18, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 Rednickle/node-1#71
Open
[Snyk] Security upgrade gatsby-plugin-sharp from 2.6.24 to 4.25.1 baby636/node#89
Open
@137717unity 137717unity mentioned this pull request Apr 18, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 137717unity/node#94
Open
@wedataintelligence wedataintelligence mentioned this pull request Apr 18, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 wedataintelligence/node#76
Open
This was referenced Apr 19, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.2.13 to 4.25.1 kissofire/node#97
Open
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 molllyn1/node#77
Open
@ron-a20 ron-a20 mentioned this pull request Apr 19, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 ron-a20/node#94
Open
This was referenced Apr 19, 2023
[Snyk] Security upgrade gatsby-plugin-sharp from 2.3.10 to 4.25.1 aliceUnhinged613/node#91
Open
[Snyk] Security upgrade gatsby from 2.24.27 to 4.25.2 baby636/node#91
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@targos targos targos approved these changes
@devsnek devsnek devsnek approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

ModuleWrap::SyntheticModuleEvaluationStepsCallback should return a Promise

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@dandclark @nodejs-github-bot @aduh95 @devsnek @ai @targos
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.