esm: Source Phase Imports for WebAssembly #56919

guybedford · 2025-02-05T02:26:03Z

This implements the Stage 3 Source Phase Imports proposal for Node.js (with latest specification at tc39/ecma262#3492), based on the new V8 parser and module loading APIs for this feature.

Phases in the module pipeline represent the ability to load uninstantiated modules (and in future with import defer, unexecuted modules as well).

While we are still waiting for progression on https://github.com/tc39/proposal-esm-phase-imports for JS Source Phase objects, this implements only the WebAssembly source phase per the Wasm ESM Integration Phase 3 proposal (https://github.com/webassembly/esm-integration).

This PR adds support both for dynamic and static source phase syntax:

import source mod1 from './mod.wasm';
const mod2 = await import.source('./mod.wasm');

This allows obtaining a Wasm module through the ESM integration, without it being instantiated through the Node.js resolver, to permit custom instantiations:

import source mod from './mod.wasm';

const instance = await WebAssembly.instantiate(mod, imports);

The dynamic source phase support is a one-line TODO, which is pending #56842. When this lands the skipped tests have been tested against that branch to fully pass.

When a source phase representation for a module is not available, a new ERR_SOURCE_PHASE_NOT_DEFINED SyntaxError is thrown as per the standard.

The VM API is updated to take a string phase argument as its last argument, although the ability to define source phase representations for virtual modules is not currently supported, and left as a future integration point. Perhaps this will simplify with ESM Phase Imports in future as well allowing handles to modules outside of the VM graph.

Resolves #53178.

@nodejs/loaders

nodejs-github-bot · 2025-02-05T02:26:09Z

Review requested:

@nodejs/loaders
@nodejs/startup
@nodejs/vm

codecov · 2025-02-05T03:59:54Z

Codecov Report

Attention: Patch coverage is 82.15488% with 53 lines in your changes missing coverage. Please review.

Project coverage is 89.09%. Comparing base (79f96b6) to head (81890a1).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	62.50%	23 Missing and 13 partials ⚠️
lib/internal/modules/esm/module_job.js	89.77%	9 Missing ⚠️
lib/internal/modules/esm/loader.js	87.50%	3 Missing and 1 partial ⚠️
lib/internal/vm/module.js	85.71%	3 Missing ⚠️
lib/repl.js	75.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #56919      +/-   ##
==========================================
- Coverage   89.10%   89.09%   -0.02%     
==========================================
  Files         665      665              
  Lines      193203   193400     +197     
  Branches    37220    37246      +26     
==========================================
+ Hits       172158   172312     +154     
- Misses      13771    13806      +35     
- Partials     7274     7282       +8

Files with missing lines	Coverage Δ
lib/internal/modules/cjs/loader.js	`98.26% <100.00%> (ø)`
lib/internal/modules/esm/translators.js	`91.51% <100.00%> (+0.06%)`	⬆️
lib/internal/modules/esm/utils.js	`98.90% <100.00%> (+0.01%)`	⬆️
lib/internal/modules/run_main.js	`97.60% <100.00%> (ø)`
lib/internal/process/execution.js	`94.79% <100.00%> (-0.16%)`	⬇️
lib/internal/vm.js	`100.00% <100.00%> (ø)`
src/module_wrap.h	`0.00% <ø> (ø)`
src/node.cc	`73.69% <100.00%> (+0.11%)`	⬆️
src/node_errors.h	`88.46% <100.00%> (+3.46%)`	⬆️
lib/repl.js	`94.90% <75.00%> (-0.05%)`	⬇️
... and 4 more

... and 23 files with indirect coverage changes

mcollina

lgtm

nodejs-github-bot · 2025-02-05T06:47:59Z

CI: https://ci.nodejs.org/job/node-test-pull-request/64999/

src/module_wrap.cc

legendecas · 2025-02-05T10:34:05Z

lib/internal/vm.js

+ * @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer
+ * @param {Record<string, string>} attributes
+ * @param {string} phase
+ * @returns { Promise<void> }


Is this return value type correct? I think it should be Promise<ModuleNamespace|vm.Module>

No it's not - but this is a move of an existing definition, so this is as it's currently written on main.

Unless we never want to fix it, we should probably fix it on main first, then move it in this PR

doc/api/errors.md

doc/api/esm.md

test/es-module/test-esm-wasm.mjs

doc/api/vm.md

lib/internal/modules/esm/module_job.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Michaël Zasso <targos@protonmail.com>

This reverts commit 009373d.

nodejs-github-bot · 2025-02-13T19:20:24Z

CI: https://ci.nodejs.org/job/node-test-pull-request/65191/

aduh95 · 2025-02-13T23:09:54Z

src/module_wrap.cc

@@ -607,8 +623,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
  }

  if (try_catch.HasCaught()) {
-    if (!try_catch.HasTerminated())
-      try_catch.ReThrow();
+    if (!try_catch.HasTerminated()) try_catch.ReThrow();


This looks like an unrelated change that's going to make backporting stuff needlessly harder, could we get rid of it?

aduh95 · 2025-02-13T23:10:00Z

src/module_wrap.cc

+    USE(module->InstantiateModule(
+        context, ResolveModuleCallback, ResolveSourceCallback));


aduh95 · 2025-02-13T23:10:15Z

src/module_wrap.cc

+  USE(module->InstantiateModule(
+      context, ResolveModuleCallback, ResolveSourceCallback));


aduh95 · 2025-02-13T23:10:21Z

src/module_wrap.cc

@@ -551,7 +567,7 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo<Value>& args) {
  ContextifyContext* contextify_context = obj->contextify_context_;
  MicrotaskQueue* microtask_queue = nullptr;
  if (contextify_context != nullptr)
-      microtask_queue = contextify_context->microtask_queue();
+    microtask_queue = contextify_context->microtask_queue();


aduh95 · 2025-02-13T23:10:35Z

src/module_wrap.cc

+  if (import_callback
+          ->Call(
+              context, Undefined(isolate), arraysize(import_args), import_args)
+          .ToLocal(&result)) {


aduh95 · 2025-02-13T23:10:48Z

src/module_wrap.cc

  Environment* env = Environment::GetCurrent(context);
-  if (env == nullptr)
-    return;
+  if (env == nullptr) return;


aduh95 · 2025-02-13T23:10:56Z

src/module_wrap.cc

+  obj->object()->SetInternalField(kSyntheticEvaluationStepsSlot,
+                                  Undefined(isolate));
+  MaybeLocal<Value> ret =
+      synthetic_evaluation_steps->Call(context, obj->object(), 0, nullptr);


bartlomieju · 2025-02-17T23:04:39Z

test/es-module/test-esm-wasm.mjs

+      '--eval',
+      [
+        'import { strictEqual } from "node:assert";',
+        `import * as wasmExports from ${JSON.stringify(fixtures.fileURL('es-modules/wasm-source-phase.js'))};`,


Drive-by; shouldn't this say import source * as wasmExports?

bartlomieju · 2025-02-17T23:06:01Z

test/es-module/test-esm-wasm.mjs

+        'import { strictEqual } from "node:assert";',
+        `import source mod from ${JSON.stringify(fixtures.fileURL('es-modules/unimportable.wasm'))};`,
+        'assert.strictEqual(mod instanceof WebAssembly.Module, true);',


Also a question here - how does this test ensures that the import is not executed?

sbc100 · 2025-02-19T19:54:00Z

I'm working on corresponding patch to emscripten to enable source phase imports: emscripten-core/emscripten#23175

As such, I would love to see this patch landed so we can start testing it!

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 5, 2025

guybedford force-pushed the source-phase-imports branch 2 times, most recently from c485531 to 8b3ce05 Compare February 5, 2025 02:31

guybedford changed the title ~~esm: Support Source Phase Imports for WebAssembly~~ esm: Source Phase Imports for WebAssembly Feb 5, 2025

mcollina approved these changes Feb 5, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 5, 2025

marco-ippolito added esm Issues and PRs related to the ECMAScript Modules implementation. wasm Issues and PRs related to WebAssembly. labels Feb 5, 2025

targos reviewed Feb 5, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

legendecas linked an issue Feb 5, 2025 that may be closed by this pull request

WebAssembly source phase imports #56056

Open

targos reviewed Feb 5, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

targos reviewed Feb 5, 2025

View reviewed changes

src/module_wrap.cc Outdated Show resolved Hide resolved

targos approved these changes Feb 5, 2025

View reviewed changes

legendecas reviewed Feb 5, 2025

View reviewed changes

aduh95 reviewed Feb 5, 2025

View reviewed changes

doc/api/errors.md Show resolved Hide resolved

aduh95 reviewed Feb 5, 2025

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

aduh95 reviewed Feb 5, 2025

View reviewed changes

doc/api/esm.md Outdated Show resolved Hide resolved

Qard approved these changes Feb 6, 2025

View reviewed changes

aduh95 reviewed Feb 6, 2025

View reviewed changes

test/es-module/test-esm-wasm.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Feb 6, 2025

View reviewed changes

test/es-module/test-esm-wasm.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Feb 6, 2025

View reviewed changes

test/es-module/test-esm-wasm.mjs Outdated Show resolved Hide resolved

aduh95 reviewed Feb 6, 2025

View reviewed changes

test/es-module/test-esm-wasm.mjs Outdated Show resolved Hide resolved

guybedford mentioned this pull request Feb 7, 2025

Source Phase Imports Implementation denoland/deno#28013

Open

aduh95 mentioned this pull request Feb 10, 2025

tools: add support for import source syntax in linter #56992

Merged

legendecas reviewed Feb 11, 2025

View reviewed changes

doc/api/vm.md Show resolved Hide resolved

lib/internal/modules/esm/module_job.js Show resolved Hide resolved

guybedford mentioned this pull request Feb 13, 2025

Source Phase Imports support unjs/unwasm#50

Open

1 task

guybedford and others added 4 commits February 13, 2025 10:07

esm: support source phase imports for WebAssembly

db8a5cc

fixup: code review

7ae0ffa

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com> Co-authored-by: Michaël Zasso <targos@protonmail.com>

add source phase eslint rule

2c5acda

add pr feedback

98b0d48

guybedford force-pushed the source-phase-imports branch from 6364566 to 98b0d48 Compare February 13, 2025 18:08

guybedford added 2 commits February 13, 2025 10:20

update to latest v8

009373d

Revert "update to latest v8"

81890a1

This reverts commit 009373d.

guybedford mentioned this pull request Feb 13, 2025

esm: unflag --experimental-wasm-modules #57038

Open

aduh95 reviewed Feb 13, 2025

View reviewed changes

bartlomieju reviewed Feb 17, 2025

View reviewed changes

dsherret mentioned this pull request Feb 18, 2025

Support source phase imports microsoft/TypeScript#61216

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: Source Phase Imports for WebAssembly #56919

esm: Source Phase Imports for WebAssembly #56919

guybedford commented Feb 5, 2025 •

edited

Loading

nodejs-github-bot commented Feb 5, 2025

codecov bot commented Feb 5, 2025 •

edited

Loading

mcollina left a comment

nodejs-github-bot commented Feb 5, 2025

legendecas Feb 5, 2025

guybedford Feb 5, 2025

aduh95 Feb 13, 2025

legendecas Feb 21, 2025

nodejs-github-bot commented Feb 13, 2025

aduh95 Feb 13, 2025

aduh95 Feb 13, 2025

aduh95 Feb 13, 2025

aduh95 Feb 13, 2025

aduh95 Feb 13, 2025

aduh95 Feb 13, 2025

aduh95 Feb 13, 2025

bartlomieju Feb 17, 2025

bartlomieju Feb 17, 2025

sbc100 commented Feb 19, 2025

		USE(module->InstantiateModule(
		context, ResolveModuleCallback, ResolveSourceCallback));

esm: Source Phase Imports for WebAssembly #56919

Are you sure you want to change the base?

esm: Source Phase Imports for WebAssembly #56919

Conversation

guybedford commented Feb 5, 2025 • edited Loading

nodejs-github-bot commented Feb 5, 2025

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodejs-github-bot commented Feb 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbc100 commented Feb 19, 2025

guybedford commented Feb 5, 2025 •

edited

Loading

codecov bot commented Feb 5, 2025 •

edited

Loading