Skip to content
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

vm importModuleDynamically option in Node 20.10 requires --experimental-vm-modules flag and 20.9 does not #51154

Closed
zachleat opened this issue Dec 14, 2023 · 7 comments · Fixed by #51244
Labels
vm Issues and PRs related to the vm subsystem.

Comments

@zachleat
Copy link

zachleat commented Dec 14, 2023

Version

v20.10.0

Platform

23.1.0 Darwin Kernel Version 23.1.0: Mon Oct 9 21:28:12 PDT 2023; root:xnu-10002.41.9~6/RELEASE_ARM64_T8103 arm64

Subsystem

node:vm

What steps will reproduce the bug?

Given the following (I also tested a CommonJS version with the same result):

import vm from "vm";

let code = `
;(async function() {
	await import("@zachleat/noop");
})()`

let context = vm.createContext({});

await vm.runInContext(code, context, {
	importModuleDynamically: function(specifier) {
		return import(specifier);
	}
});

How often does it reproduce? Is there a required condition?

Throws an error every time on Node v20.10 and newer. Both in ESM and CJS versions of the test code.

What is the expected behavior? Why is that the expected behavior?

Previous versions of Node prior to v20.10 did not require the --experimental-vm-modules flag. If import() is supported in CommonJS—why is import() not supported in vm? I was relying on this method as an escape hatch until vm.Module was stable. I suppose my question is: was this a bug that was fixed or is this a regression?

Failures

Node v20.10 and newer: `node reduced-test.js`
Node v20.10 and newer: `node reduced-test.cjs`

Successes

Node v20.10 and newer: `node --experimental-vm-modules reduced-test.js`
Node v20.10 and newer: `node --experimental-vm-modules reduced-test.cjs`
Node v14–v20.9: `node --experimental-vm-modules reduced-test.js`
Node v14–v20.9: `node --experimental-vm-modules reduced-test.cjs`
Node v14–v20.9: `node reduced-test.js`
Node v14–v20.9: `node reduced-test.cjs`

What do you see instead?

node:internal/modules/esm/utils:180
    throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG();
          ^

TypeError [ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG]: A dynamic import callback was invoked without --experimental-vm-modules
    at importModuleDynamicallyCallback (node:internal/modules/esm/utils:180:11)
    at evalmachine.<anonymous>:3:2
    at evalmachine.<anonymous>:4:3
    at Script.runInContext (node:vm:133:12)
    at Object.runInContext (node:vm:279:6)
    at file:///Users/zachleat/Code/node-retrieve-globals/reduced-test.js:10:20
    at ModuleJob.run (node:internal/modules/esm/module_job:218:25)
    at async ModuleLoader.import (node:internal/modules/esm/loader:329:24)
    at async loadESM (node:internal/process/esm_loader:34:7)
    at async handleMainPromise (node:internal/modules/run_main:113:12) {
  code: 'ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG'
}

Node.js v20.10.0

Additional information

Appreciate y’all!

@legendecas
Copy link
Member

legendecas commented Dec 14, 2023

Related: #49950 /cc @joyeecheung

I believe we can lift the restriction at https://github.com/nodejs/node/blob/main/lib/internal/vm.js#L50-L58. When importModuleDynamically callback is set, a ModuleNamespace which is available without --experimental-vm-modules can be returned instead. While importModuleDynamically callback is not set, the compilation cache will still be valid with default_host_defined_options.

@joyeecheung
Copy link
Member

This is an intentional change to address what this comment describes:

node/lib/internal/vm.js

Lines 50 to 58 in 99f6084

// We should've thrown here immediately when we introduced
// --experimental-vm-modules and importModuleDynamically, but since
// users are already using this callback to throw a similar error,
// we also defer the error to the time when an actual import() is called
// to avoid breaking them. To ensure that the isolate compilation
// cache can still be hit, use a constant sentinel symbol here.
if (!getOptionValue('--experimental-vm-modules')) {
return vm_dynamic_import_missing_flag;
}

See #49950 (comment) for background. The callback has always been described as:

This option is part of the experimental modules API. We do not recommend using it in a production environment.

in the documentation, it's more of a negligence to allow it without --experimental-vm-modules.

@joyeecheung
Copy link
Member

On a side note I expect us to revamp the design a bit and move this callback to module evaluation/script execution time instead of compilation time, once V8 finishes https://bugs.chromium.org/p/v8/issues/detail?id=10284 (which is now moving again). I think it's good to make it clear that it's still experimental to reduce the dependency on the flawed design.

@zachleat
Copy link
Author

zachleat commented Dec 14, 2023

Ah, just to be super clear—the intention here is to disallow all use of import() in vm when --experimental-vm-modules is not set? I can shim in my own require but it’s not possible to shim import now in Node v20.10+—this means I will not be able to use any external ESM npm packages in vm until vm.Module is stable.

Alternatively (just thinking out loud), I could 1. use a bundler like esbuild to preprocess the code or 2. separately dynamically import them outside of the script and inject them to the context manually.

@joyeecheung
Copy link
Member

joyeecheung commented Dec 14, 2023

The experimental status is specifically for customization. Though for your use case or, just as a utility for customization-less import in general, we can also add an option to proxy all the dynamic import within a vm-compiled script to the default loader. (I would still mark that as experimental, but it can emit a warning instead of throwing an error).

@joyeecheung
Copy link
Member

Opened #51244 to support this fallback via importModuleDynamically: vm.constants.USE_MAIN_CONTEXT_DEFAULT_LOADER

@zachleat
Copy link
Author

Yay—you’re amazing. Thank you!!

nodejs-github-bot pushed a commit that referenced this issue Feb 1, 2024
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: #51244
Fixes: #51154
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
rdw-msft pushed a commit to rdw-msft/node that referenced this issue Feb 9, 2024
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#51244
Fixes: nodejs#51154
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
targos pushed a commit that referenced this issue Feb 15, 2024
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: #51244
Fixes: #51154
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this issue Feb 19, 2024
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#51244
Fixes: nodejs#51154
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
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: #51244
Fixes: #51154
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
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: #51244
Fixes: #51154
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vm Issues and PRs related to the vm subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants