-
Notifications
You must be signed in to change notification settings - Fork 30.5k
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
esm: Source Phase Imports for WebAssembly #56919
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
aa33f53
to
c485531
Compare
c485531
to
8b3ce05
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #56919 +/- ##
==========================================
- Coverage 89.18% 89.15% -0.04%
==========================================
Files 665 665
Lines 192543 192771 +228
Branches 37045 37113 +68
==========================================
+ Hits 171726 171865 +139
- Misses 13636 13687 +51
- Partials 7181 7219 +38
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
* @param {ModuleWrap|ContextifyScript|Function|vm.Module} callbackReferrer | ||
* @param {Record<string, string>} attributes | ||
* @param {string} phase | ||
* @returns { Promise<void> } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this return value type correct? I think it should be Promise<ModuleNamespace|vm.Module>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not - but this is a move of an existing definition, so this is as it's currently written on main.
For example, to create multiple instances of a module, or to pass custom imports | ||
into a new instance of `library.wasm`: | ||
<!-- eslint-skip --> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we instead add the required plugin? Can happen in a follow-up PR
diff --git a/eslint.config.mjs b/eslint.config.mjs
index ef195d0c60..912d326557 100644
--- a/eslint.config.mjs
+++ b/eslint.config.mjs
@@ -17,6 +17,7 @@ import nodeCore from './tools/eslint/eslint-plugin-node-core.js';
const js = requireEslintTool('@eslint/js');
const babelEslintParser = requireEslintTool('@babel/eslint-parser');
const babelPluginSyntaxImportAttributes = resolveEslintTool('@babel/plugin-syntax-import-attributes');
+const babelPluginSyntaxImportSource = resolveEslintTool('@babel/plugin-syntax-import-source');
const jsdoc = requireEslintTool('eslint-plugin-jsdoc');
const markdown = requireEslintTool('eslint-plugin-markdown');
const stylisticJs = requireEslintTool('@stylistic/eslint-plugin-js');
@@ -74,6 +75,7 @@ export default [
babelOptions: {
plugins: [
babelPluginSyntaxImportAttributes,
+ babelPluginSyntaxImportSource,
],
},
requireConfigFile: false,
diff --git a/tools/eslint/package.json b/tools/eslint/package.json
index 68bedee0cb..0b6244654e 100644
--- a/tools/eslint/package.json
+++ b/tools/eslint/package.json
@@ -6,6 +6,7 @@
"@babel/core": "^7.26.0",
"@babel/eslint-parser": "^7.25.9",
"@babel/plugin-syntax-import-attributes": "^7.26.0",
+ "@babel/plugin-syntax-import-source": "^7.25.9",
"@stylistic/eslint-plugin-js": "^2.12.1",
"eslint": "^9.17.0",
"eslint-formatter-tap": "^8.40.0",
Co-authored-by: Antoine du Hamel <[email protected]> Co-authored-by: Michaël Zasso <[email protected]>
'await m1.evaluate();', | ||
].join('\n'), | ||
]); | ||
ok(stderr.includes('Source phase import object is not defined for module')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To get better error output
ok(stderr.includes('Source phase import object is not defined for module')); | |
match(stderr, /Source phase import object is not defined for module/); |
'--eval', | ||
`import source nosource from ${JSON.stringify(fileUrl)};`, | ||
]); | ||
ok(stderr.includes('Source phase import object is not defined for module')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok(stderr.includes('Source phase import object is not defined for module')); | |
match(stderr, /Source phase import object is not defined for module/); |
'await m1.evaluate();', | ||
].join('\n'), | ||
]); | ||
ok(stderr.includes('Source phase import object is not defined for module')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok(stderr.includes('Source phase import object is not defined for module')); | |
match(stderr, /Source phase import object is not defined for module/); |
[ | ||
'import { ok, strictEqual } from "node:assert";', | ||
'try {', | ||
` await import.source(${JSON.stringify(fileUrl)});`, | ||
' ok(false);', | ||
'}', | ||
'catch (e) {', | ||
' strictEqual(e instanceof SyntaxError, true);', | ||
' strictEqual(e.message.includes("Source phase import object is not defined for module"), true);', | ||
` strictEqual(e.message.includes(${JSON.stringify(fileUrl)}), true);`, | ||
'}', | ||
].join('\n'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[ | |
'import { ok, strictEqual } from "node:assert";', | |
'try {', | |
` await import.source(${JSON.stringify(fileUrl)});`, | |
' ok(false);', | |
'}', | |
'catch (e) {', | |
' strictEqual(e instanceof SyntaxError, true);', | |
' strictEqual(e.message.includes("Source phase import object is not defined for module"), true);', | |
` strictEqual(e.message.includes(${JSON.stringify(fileUrl)}), true);`, | |
'}', | |
].join('\n'), | |
`await assert.rejects(import.source(${JSON.stringify(fileUrl)}), (e) => { | |
assert.match(e.message, /Source phase import object is not defined for module/); | |
assert.ok(e.message.includes(${JSON.stringify(fileUrl)})); | |
assert.ok(e instanceof SyntaxError); | |
});` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review, I'll aim to incorporate the changes soon. For this rewriting, I was just following the pattern already set in this test - I think if we want to change this we should consider doing it for the whole file, which would be a larger rewriting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which test? I'd be up to do that rewriting, but I can't seem to find where we use that pattern
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:
This allows obtaining a Wasm module through the ESM integration, without it being instantiated through the Node.js resolver, to permit custom instantiations:
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