Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ModuleWrap::SyntheticModuleEvaluationStepsCallback should return a Promise #37299

Closed
dandclark opened this issue Feb 9, 2021 · 0 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.

Comments

@dandclark
Copy link
Contributor

Top-level await expects that all module script evaluation returns a Promise. As such, ModuleWrap::SyntheticModuleEvaluationStepsCallback should be updated 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 Stage 1 Built-in modules proposal isn't yet updated for top-level await.

For reference, the corresponding change for Blink is https://chromium-review.googlesource.com/c/chromium/src/+/2568823.

I discovered this issue when working on this V8 bugfix: https://chromium-review.googlesource.com/c/v8/v8/+/2673794. My first attempt at a fix failed the Node integration tests because it assumed that the Synthetic Module callback steps return a Promise. For now, I'm adding a workaround for this in V8 but if Node can make this update then we'd like to eventually remove that workaround.

dandclark added a commit to dandclark/node that referenced this issue Feb 9, 2021
…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
@devsnek devsnek added confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features. labels Feb 9, 2021
danielleadams pushed a commit that referenced this issue Feb 16, 2021
…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]>
targos pushed a commit that referenced this issue May 1, 2021
…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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. esm Issues and PRs related to the ECMAScript Modules implementation. experimental Issues and PRs related to experimental features.
Projects
None yet
2 participants