vm: support using the default loader to handle dynamic import()#51244

joyeecheung · 2023-12-21T00:58:25Z

This patch adds support for using vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER as the
importModuleDynamically option in all vm APIs that take this option except vm.SourceTextModule. This allows users to have a shortcut to support dynamic import() in the compiled code without missing the compilation cache if they don't need customization of the loading process. We emit an experimental warning when the import() is actually handled by the default loader through this option instead of requiring --experimental-vm-modules.

const{ Script, constants }=require('node:vm');const{ resolve }=require('node:path');const{ writeFileSync }=require('node:fs');// Write test.js and test.txt to the directory where the current script// being run is located.writeFileSync(resolve(__dirname,'test.mjs'),'export const filename = "./test.json";');writeFileSync(resolve(__dirname,'test.json'),'{"hello": "world"}');// Compile a script that loads test.mjs and then test.json// as if the script is placed in the same directory.constscript=newScript(`(async function(){ const{filename } = await import('./test.mjs'); return import(filename,{with:{type: 'json' } }) })();`,{filename: resolve(__dirname,'test-with-default.js'),importModuleDynamically: constants.USE_MAIN_CONTEXT_DEFAULT_LOADER,});//{default:{hello: 'world' }}script.runInThisContext().then(console.log);

In addition this refactors the documentation for
importModuleDynamically and adds a dedicated section for it with examples.

vm.SourceTextModule is not supported in this patch because it needs additional refactoring to handle initializeImportMeta, which can be done in a follow-up.

Fixes: #51154

nodejs-github-bot · 2023-12-21T00:58:30Z

Review requested:

@nodejs/loaders
@nodejs/vm
@nodejs/web-infra

nodejs-github-bot · 2023-12-21T01:23:32Z

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

GeoffreyBooth

A few small things, but mostly looks great! Thank you for doing this. The last time I tried to use --experimental-vm-modules I was overwhelmed by the amount of boilerplate required; I’m glad to see a lot of that going away.

doc/api/vm.md

GeoffreyBooth · 2023-12-21T04:36:34Z

doc/api/vm.md

+constscript=newScript('import("./foo.json",{with:{type: "json"} })',{
+ filename:resolve(import.meta.dirname, 'test-with-default.js'),
+ importModuleDynamically:constants.USE_MAIN_CONTEXT_DEFAULT_LOADER,
+});


This seems like it’s a CommonJS script? But it’s a bit of an awkward example since in CommonJS most users would use require to get a JSON file, rather than dynamic import. Maybe change it to a dynamic import of an ES module, with a comment above like // Import ES module into CommonJS code?
I assume we can’t show how this would be used with an ESM script because this doesn’t yet support SourceTextModule?

This is not a CommonJS script, vm.Script is just a script. If you run it in a new context, there is no require() at all (there also isn't going to be process, or URL, or anything not implemented by V8).

GeoffreyBooth · 2023-12-21T04:38:48Z

doc/api/vm.md

+`filename` that's either an absolute path or a URL string. If `filename` is
+ a string that's neither an absolute path or a URL string, or if it's


URL string, or URL instance? Generally we don’t allow a path to be either a path string or an URL string, because of ambiguity. #48994

Only strings are allowed as resource names, passing URL instances result in an error, this is the compilation API where the name is going to be used for stack traces and must be primitives.

Also note that this is not necessarily a path, it can be a http:// URL. As long as import('file:///path/to/foo') is allowed by the default loader, this is allowed. Whenever that becomes disallowed by the default loader, the default loader can throw, and this documentation can be updated.

doc/api/vm.md

lib/internal/modules/esm/utils.js

GeoffreyBooth · 2023-12-21T04:45:42Z

lib/internal/modules/helpers.js

 }
-returnnewURL(referrer).href;
+
+if(StringPrototypeStartsWith(referrerName,'file://')){


We can’t do this. I proposed this in #48994 but it was shot down because you can, in fact, have a path that begins with file://; and it’s actually not all that uncommon, when people mirror URLs in a local filesystem. The logic needs to be “if it’s an instance of the URL class, parse as URL; else parse strings as paths.” cc @aduh95

We do not accept URL in the API, only the strings. This is the current behavior and disallowing file:// paths is semver major (because the filename is part of the stable API).
Also note that only strings or other primitives can end up here after the hands of V8. The default loader can also choose to throw for a file:/// string in the future, and we can update the docs when that happens. For now this is just mimic-ing the existing behavior.

Updated to simply return the string as-is instead of parsing it at all when it looks like a URL. However the default loader treat a module with a file:// URL string name is up to the default loader, we are only passing the name over.

lib/internal/process/pre_execution.js

GeoffreyBooth · 2023-12-21T04:49:49Z

lib/internal/source_map/source_map_cache.js

 functionmaybeCacheSourceMap(filename,content,cjsModuleInstance,isGeneratedSource,sourceURL,sourceMapURL){
 constsourceMapsEnabled=getSourceMapsEnabled();
 if(!(process.env.NODE_V8_COVERAGE||sourceMapsEnabled))return;
-try{
-const{ normalizeReferrerURL }=require('internal/modules/helpers');
-filename=normalizeReferrerURL(filename);
-}catch(err){
+const{ normalizeReferrerURL }=require('internal/modules/helpers');
+filename=normalizeReferrerURL(filename);
+if(filename===undefined){


This function could really use a JSDoc, if you know what it should be. What is filename? Is it a path string? Because normalizeReferrerURL returns an URL string; shouldn’t it be one or the other?
This feature should support data URLs and --experimental-network-imports, so path strings should probably be normalized to URL strings so that non-file:// URLs work.

This function could really use a JSDoc, if you know what it should be.
I don't know fully what this does but filename is supposed to be always a string because it's used as keys to SafeMaps below.
shouldn’t it be one or the other?
The current behavior is that "it doesn't matter as long as this string identifies a module that contains the sourceURL that V8 gives back to us". I think there can be some cache misses caused by this but that's probably out of scope of this PR, because this PR just tries to preserve the existing mapping behavior in normalizeReferrerURL.
This feature should support data URLs and --experimental-network-imports, so path strings should probably be normalized to URL strings so that non-file:// URLs work.
Supporting those for source maps seems to be out of scope for this PR? Again this is just preserving the existing behavior. Whether additional normalization should happen and how that should work with other components should probably be discussed elsewhere.

The filename here can either be an absolute path string (for CJS) or a URL string (for ESM) and is normalized to a URL string by normalizeReferrerURL. I can add JSDoc for source map related functions in a follow-up PR.

doc/api/vm.md

test/es-module/test-vm-main-context-default-loader.js

lib/internal/modules/helpers.js

doc/api/vm.md

nodejs-github-bot · 2023-12-21T18:53:20Z

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

joyeecheung · 2024-01-08T17:10:57Z

Ping @GeoffreyBooth did I answer your question about URL strings? The vm APIs only allows strings as identifiers and after the hands of V8 the resource names must be strings. I don't think URL is that big of an issue for this change. Whatever the vm API supports, it will continue to support. Disallowing file:// strings for resource names in vm APIs would be a semver-major that should be discussed elsewhere.

doc/api/vm.md

lib/internal/process/pre_execution.js

This patch adds support for using `vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER` as `importModuleDynamically` in all APIs that take the option except `vm.SourceTextModule`. This allows users to have a shortcut to support dynamic import() in the compiled code without missing the compilation cache if they don't need customization of the loading process. We emit an experimental warning when the `import()` is actually handled by the default loader through this option instead of requiring `--experimental-vm-modules`. In addition this refactors the documentation for `importModuleDynamically` and adds a dedicated section for it with examples. `vm.SourceTextModule` is not supported in this patch because it needs additional refactoring to handle `initializeImportMeta`, which can be done in a follow-up.

This patch adds support for using `vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER` as `importModuleDynamically` in all APIs that take the option except `vm.SourceTextModule`. This allows users to have a shortcut to support dynamic import() in the compiled code without missing the compilation cache if they don't need customization of the loading process. We emit an experimental warning when the `import()` is actually handled by the default loader through this option instead of requiring `--experimental-vm-modules`. In addition this refactors the documentation for `importModuleDynamically` and adds a dedicated section for it with examples. `vm.SourceTextModule` is not supported in this patch because it needs additional refactoring to handle `initializeImportMeta`, which can be done in a follow-up. PR-URL: nodejs#51244Fixes: nodejs#51154 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add zcbenz to collaborators (Cheng Zhao) #51812 * add lemire to collaborators (Daniel Lemire) #51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) #51412 * (SEMVER-MINOR) add server handshake utility (snek) #51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) #51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) #51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) #51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) #51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) #50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: TODO

Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add zcbenz to collaborators (Cheng Zhao) #51812 * add lemire to collaborators (Daniel Lemire) #51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) #51412 * (SEMVER-MINOR) add server handshake utility (snek) #51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) #51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) #51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) #51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) #51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) #50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #51932

This patch adds support for using `vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER` as `importModuleDynamically` in all APIs that take the option except `vm.SourceTextModule`. This allows users to have a shortcut to support dynamic import() in the compiled code without missing the compilation cache if they don't need customization of the loading process. We emit an experimental warning when the `import()` is actually handled by the default loader through this option instead of requiring `--experimental-vm-modules`. In addition this refactors the documentation for `importModuleDynamically` and adds a dedicated section for it with examples. `vm.SourceTextModule` is not supported in this patch because it needs additional refactoring to handle `initializeImportMeta`, which can be done in a follow-up. PR-URL: #51244Fixes: #51154 Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]>

Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add lemire to collaborators (Daniel Lemire) #51572 * add zcbenz to collaborators (Cheng Zhao) #51812 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #52212

Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) nodejs#51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) nodejs#51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) nodejs#51794 doc: * add zcbenz to collaborators (Cheng Zhao) nodejs#51812 * add lemire to collaborators (Daniel Lemire) nodejs#51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) nodejs#51412 * (SEMVER-MINOR) add server handshake utility (snek) nodejs#51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) nodejs#51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) nodejs#51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) nodejs#50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) nodejs#51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) nodejs#51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) nodejs#51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) nodejs#50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) nodejs#51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) nodejs#51244 PR-URL: nodejs#51932

Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) #51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) #51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) #51794 doc: * add lemire to collaborators (Daniel Lemire) #51572 * add zcbenz to collaborators (Cheng Zhao) #51812 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) #51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) #50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) #50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) #50960 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) #51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) #51244 PR-URL: #52212

Notable changes: build: * (SEMVER-MINOR) build opt to set local location of headers (Michael Dawson) nodejs#51525 crypto: * (SEMVER-MINOR) implement crypto.hash() (Joyee Cheung) nodejs#51044 * update root certificates to NSS 3.98 (Node.js GitHub Bot) nodejs#51794 doc: * add zcbenz to collaborators (Cheng Zhao) nodejs#51812 * add lemire to collaborators (Daniel Lemire) nodejs#51572 http2: * (SEMVER-MINOR) add h2 compat support for appendHeader (Tim Perry) nodejs#51412 * (SEMVER-MINOR) add server handshake utility (snek) nodejs#51172 * (SEMVER-MINOR) receive customsettings (Marten Richter) nodejs#51323 lib: * (SEMVER-MINOR) move encodingsMap to internal/util (Joyee Cheung) nodejs#51044 sea: * (SEMVER-MINOR) support sea.getRawAsset() (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support embedding assets (Joyee Cheung) nodejs#50960 src: * (SEMVER-MINOR) print string content better in BlobDeserializer (Joyee Cheung) nodejs#50960 * (SEMVER-MINOR) support multi-line values for .env file (IlyasShabi) nodejs#51289 * (SEMVER-MINOR) add `process.loadEnvFile` and `util.parseEnv` (Yagiz Nizipli) nodejs#51476 * (SEMVER-MINOR) do not coerce dotenv paths (Tobias Nießen) nodejs#51425 stream: * (SEMVER-MINOR) implement `min` option for `ReadableStreamBYOBReader.read` (Mattias Buelens) nodejs#50888 util: * (SEMVER-MINOR) add styleText API to text formatting (Rafael Gonzaga) nodejs#51850 vm: * (SEMVER-MINOR) support using the default loader to handle dynamic import() (Joyee Cheung) nodejs#51244 PR-URL: nodejs#51932

Current alignment mislead doc reader into thinking `importModuleDynamically` is a separate positional param right next to `options`, which is incorrect and need to be fixed. This misalignment is introduced in a PR merged in Feb 2024. I belive this doc fix applies to node v20 and above. Refs: nodejs#51244

Current alignment mislead doc reader into thinking `importModuleDynamically` is a separate positional param right next to `options`, which is incorrect and need to be fixed. This misalignment is introduced in a PR merged in Feb 2024. I belive this doc fix applies to node v20 and above. Refs: #51244 PR-URL: #58145 Reviewed-By: Darshan Sen <[email protected]> Reviewed-By: Daeyeon Jeong <[email protected]>

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 Dec 21, 2023

joyeecheung force-pushed the vm-callback-default branch from 39284fc to 5903aa9Compare December 21, 2023 00:59

joyeecheung mentioned this pull request Dec 21, 2023
vm importModuleDynamically option in Node 20.10 requires --experimental-vm-modules flag and 20.9 does not #51154
Closed

joyeecheung force-pushed the vm-callback-default branch from 5903aa9 to 5b2655dCompare December 21, 2023 01:05

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2023

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 21, 2023

joyeecheung added the semver-minor PRs that contain new features and should be released in the next minor version. label Dec 21, 2023

GeoffreyBooth reviewed Dec 21, 2023
View reviewed changes

targos reviewed Dec 21, 2023
View reviewed changes

doc/api/vm.md Outdated Show resolvedHide resolved