Uh oh!
There was an error while loading. Please reload this page.
module: change return type for ModuleWrap::SyntheticModuleEvaluationStepsCallback - #65375
module: change return type for ModuleWrap::SyntheticModuleEvaluationStepsCallback#65375caiolima 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>
nodejs-github-bot
commented
Aug 18, 2026
Review requested:
|
| V8_DEPRECATE_SOON( | ||
| "Use the CreateSyntheticModule overload whose evaluation_steps return a " | ||
| "MaybeLocal<Promise>") | ||
| static Local<Module> CreateSyntheticModule( |
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.
| } | ||
| START_ALLOW_USE_DEPRECATED() | ||
| Local<Module> Module::CreateSyntheticModule( |
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:
|
caiolima
commented
Aug 18, 2026
As a reference, this is the change on Blink without the need of backports: https://chromium-review.googlesource.com/c/chromium/src/+/8257345 |
| * 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); |
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
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.
nodejs-github-bot
commented
Aug 18, 2026
joyeecheung
commented
Sep 3, 2026
@caiolima Another commit has bumped the V8 embedder version, can you rebase and resolve the merge conflict? |
caiolima
commented
Sep 3, 2026
Sure. I'll do it. Thanks for the heads up. |
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.