module: change return type for ModuleWrap::SyntheticModuleEvaluationStepsCallback - #65375
Open
caiolima wants to merge 3 commits into
Open
module: change return type for ModuleWrap::SyntheticModuleEvaluationStepsCallback#65375caiolima wants to merge 3 commits into
caiolima wants to merge 3 commits into
Conversation
Original commit message:
[api][module] Return a Promise from Module::Evaluate()
Module evaluation always produces a Promise, but the public API exposed
the result as MaybeLocal<Value>, and for synthetic modules it could even
hand back the raw, non-Promise value returned by the embedder's
evaluation steps. Embedders therefore had to defensively cast the
result, and the actual contract was invisible in the type system.
This CL makes the contract explicit:
- SyntheticModule::Evaluate() now returns the top-level capability
Promise instead of the raw value produced by the evaluation steps, so
the completion value is always a Promise, matching source text
modules and the documented behavior.
- SyntheticModule::Evaluate() nows CHECK if callback result from
EvaluationSteps is a Promise. The reasoning behind it is that
the behavior for an embedder that relies on non-promise result
was already inconsistent, given the first call would return the
result form callback (an arbitrary Local<Value>), but subsequent
calls for module->Evaluate() would return the capability with
`undefined` as result.
- Module::Evaluate() and their variant now returns
`MaybeDirectHandle<JSPromise>` to be more explicit by the return
type.
Bug: 531396274
Change-Id: Ifbfed4f849bb47d8db1e022bfb750d9ad2d6308b
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8131138
Reviewed-by: Camillo Bruni <cbruni@chromium.org>
Commit-Queue: Caio Lima <caiolima@igalia.com>
Reviewed-by: Olivier Flückiger <olivf@chromium.org>
Cr-Commit-Position: refs/heads/main@{#109010}
Refs: v8/v8@e778593
Co-authored-by: Caio Lima <caiolima@igalia.com>
Original commit message:
[api][module] Type-check synthetic module evaluation steps
Synthetic module evaluation steps are required to return a Promise,
which becomes the module's top-level capability. Their signature
returned a MaybeLocal<Value> though, so that requirement was only
enforced by a CHECK in SyntheticModule::Evaluate().
SyntheticModuleEvaluationSteps now returns a MaybeLocal<Promise>, with a
matching CreateSyntheticModule() overload. The old signature remains
available as LegacySyntheticModuleEvaluationSteps so that embedders can
be migrated in a separate CL; it will be
deprecated and then removed once embedders migrate.
d8 and the existing tests move to the Promise-returning version,
with one cctest checking the legacy version.
Bug: 545375591
Change-Id: Id55db730678455f81394bf8a68f66f93d22f5547
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/8223168
Reviewed-by: Olivier Flückiger <olivf@chromium.org>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Commit-Queue: Caio Lima <caiolima@igalia.com>
Cr-Commit-Position: refs/heads/main@{#109273}
Refs: v8/v8@970d651
Co-authored-by: Caio Lima <caiolima@igalia.com>
… MaybeLocal<Promise>
Collaborator
|
Review requested:
|
nodejs-github-bot
added
lib / src
caiolima
commented
Aug 18, 2026
| V8_DEPRECATE_SOON( | ||
| "Use the CreateSyntheticModule overload whose evaluation_steps return a " | ||
| "MaybeLocal<Promise>") | ||
| static Local<Module> CreateSyntheticModule( |
Contributor
Author
There was a problem hiding this comment.
This becomes dead-code here once we migrate ModuleWrap::SyntheticModuleEvaluationStepsCallback to return MaybeLocal<Promise>. I'm keeping it here just to have minimal difference from original V8 commit.
caiolima
commented
Aug 18, 2026
| } | ||
|
|
||
| START_ALLOW_USE_DEPRECATED() | ||
| Local<Module> Module::CreateSyntheticModule( |
Contributor
Author
There was a problem hiding this comment.
ditto about dead-code.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #65375 +/- ##
=======================================
Coverage 90.11% 90.12%
=======================================
Files 752 752
Lines 251820 251861 +41
Branches 47351 47363 +12
=======================================
+ Hits 226923 226982 +59
- Misses 16223 16226 +3
+ Partials 8674 8653 -21
🚀 New features to boost your workflow:
|
Contributor
Author
|
As a reference, this is the change on Blink without the need of backports: https://chromium-review.googlesource.com/c/chromium/src/+/8257345 |
joyeecheung
reviewed
Aug 18, 2026
| * If IsGraphAsync() is false, the returned Promise is settled. | ||
| */ | ||
| V8_WARN_UNUSED_RESULT MaybeLocal<Value> Evaluate(Local<Context> context); | ||
| V8_WARN_UNUSED_RESULT MaybeLocal<Promise> Evaluate(Local<Context> context); |
Member
There was a problem hiding this comment.
This would break the ABI - I think if we want to backport to 22-26, this would need to be a duplicate method with a different name. Otherwise this needs to be dont-land-on-v26.x etc. and mostly just expediting things a bit more over #65161
Contributor
Author
There was a problem hiding this comment.
I'm not planning to backport, given it doesn't change pretty much anything in practice. The goal here is to make sure that we will be able to remove the callback that returns MaybeLocal<Value> eventually.
joyeecheung
added
dont-land-on-v24.x
joyeecheung
approved these changes
Aug 18, 2026
github-actions
Bot
removed
the
request-ci
Collaborator
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR doesn't change any behavior, and the motivation is to properly align with new V8 Synthetic Module API changes introduced by https://chromium-review.googlesource.com/c/v8/v8/+/8131138 and https://chromium-review.googlesource.com/c/v8/v8/+/8223168. The core of this change is to make clear on return type of
SyntheticModuleEvaluationStepsthat it should be aMaybeLocal<Promise>instead ofMaybeLocal<Value>. The latter API will be removed soon from V8.The PR is including the backport of both commits mentioned above and they are combined here to allow the change on Node side without causing compilation issues. Current code already always return a Promise, so the effective change is just the return type for
ModuleWrap::SyntheticModuleEvaluationStepsCallback.