vm: add support for import assertions in dynamic imports

aduh95 · targos · commit 4116b6c907bc · 2021-10-13T10:20:46.000+02:00
PR-URL: #40249 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
diff --git a/doc/api/vm.md b/doc/api/vm.md
@@ -54,6 +54,10 @@ executed in specific contexts.
 <!-- YAML
 added: v0.3.1
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40249
+    description: Added support for import assertions to the
+                 `importModuleDynamically` parameter.
   - version: v10.6.0
     pr-url: https://github.com/nodejs/node/pull/20300
     description: The `produceCachedData` is deprecated in favour of
@@ -91,6 +95,9 @@ changes:
     using it in a production environment.
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
+    * `importAssertions` {Object} The `"assert"` value passed to the
+      [`optionsExpression`][] optional parameter, or an empty object if no value
+      was provided.
     * Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
       recommended in order to take advantage of error tracking, and to avoid
       issues with namespaces that contain `then` function exports.
@@ -642,6 +649,13 @@ The `vm.SourceTextModule` class provides the [Source Text Module Record][] as
 defined in the ECMAScript specification.
 
 ### `new vm.SourceTextModule(code[, options])`
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40249
+    description: Added support for import assertions to the
+                 `importModuleDynamically` parameter.
+-->
 
 * `code` {string} JavaScript Module code to parse
 * `options`
@@ -667,6 +681,9 @@ defined in the ECMAScript specification.
     `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][].
     * `specifier` {string} specifier passed to `import()`
     * `module` {vm.Module}
+    * `importAssertions` {Object} The `"assert"` value passed to the
+      [`optionsExpression`][] optional parameter, or an empty object if no value
+      was provided.
     * Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
       recommended in order to take advantage of error tracking, and to avoid
       issues with namespaces that contain `then` function exports.
@@ -852,6 +869,10 @@ const vm = require('vm');
 <!-- YAML
 added: v10.10.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40249
+    description: Added support for import assertions to the
+                 `importModuleDynamically` parameter.
   - version: v15.9.0
     pr-url: https://github.com/nodejs/node/pull/35431
     description: Added `importModuleDynamically` option again.
@@ -893,6 +914,9 @@ changes:
     considered stable.
     * `specifier` {string} specifier passed to `import()`
     * `function` {Function}
+    * `importAssertions` {Object} The `"assert"` value passed to the
+      [`optionsExpression`][] optional parameter, or an empty object if no value
+      was provided.
     * Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
       recommended in order to take advantage of error tracking, and to avoid
       issues with namespaces that contain `then` function exports.
@@ -1068,6 +1092,10 @@ vm.measureMemory({ mode: 'detailed', execution: 'eager' })
 <!-- YAML
 added: v0.3.1
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40249
+    description: Added support for import assertions to the
+                 `importModuleDynamically` parameter.
   - version: v6.3.0
     pr-url: https://github.com/nodejs/node/pull/6635
     description: The `breakOnSigint` option is supported now.
@@ -1113,6 +1141,9 @@ changes:
     using it in a production environment.
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
+    * `importAssertions` {Object} The `"assert"` value passed to the
+      [`optionsExpression`][] optional parameter, or an empty object if no value
+      was provided.
     * Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
       recommended in order to take advantage of error tracking, and to avoid
       issues with namespaces that contain `then` function exports.
@@ -1145,6 +1176,10 @@ console.log(contextObject);
 <!-- YAML
 added: v0.3.1
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40249
+    description: Added support for import assertions to the
+                 `importModuleDynamically` parameter.
   - version: v14.6.0
     pr-url: https://github.com/nodejs/node/pull/34023
     description: The `microtaskMode` option is supported now.
@@ -1211,6 +1246,9 @@ changes:
     using it in a production environment.
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
+    * `importAssertions` {Object} The `"assert"` value passed to the
+      [`optionsExpression`][] optional parameter, or an empty object if no value
+      was provided.
     * Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
       recommended in order to take advantage of error tracking, and to avoid
       issues with namespaces that contain `then` function exports.
@@ -1247,6 +1285,10 @@ console.log(contextObject);
 <!-- YAML
 added: v0.3.1
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/40249
+    description: Added support for import assertions to the
+                 `importModuleDynamically` parameter.
   - version: v6.3.0
     pr-url: https://github.com/nodejs/node/pull/6635
     description: The `breakOnSigint` option is supported now.
@@ -1290,6 +1332,9 @@ changes:
     using it in a production environment.
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
+    * `importAssertions` {Object} The `"assert"` value passed to the
+      [`optionsExpression`][] optional parameter, or an empty object if no value
+      was provided.
     * Returns: {Module Namespace Object|vm.Module} Returning a `vm.Module` is
       recommended in order to take advantage of error tracking, and to avoid
       issues with namespaces that contain `then` function exports.
@@ -1452,6 +1497,7 @@ are not controllable through the timeout either.
 [`Error`]: errors.md#class-error
 [`URL`]: url.md#class-url
 [`eval()`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval
+[`optionsExpression`]: https://tc39.es/proposal-import-assertions/#sec-evaluate-import-call
 [`script.runInContext()`]: #scriptrunincontextcontextifiedobject-options
 [`script.runInThisContext()`]: #scriptruninthiscontextoptions
 [`url.origin`]: url.md#urlorigin
diff --git a/lib/internal/process/esm_loader.js b/lib/internal/process/esm_loader.js
@@ -1,5 +1,9 @@
 'use strict';
 
+const {
+  ObjectCreate,
+} = primordials;
+
 const {
   ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING,
 } = require('internal/errors').codes;
@@ -22,13 +26,14 @@ exports.initializeImportMetaObject = function(wrap, meta) {
   }
 };
 
-exports.importModuleDynamicallyCallback = async function(wrap, specifier) {
+exports.importModuleDynamicallyCallback =
+async function importModuleDynamicallyCallback(wrap, specifier, assertions) {
   const { callbackMap } = internalBinding('module_wrap');
   if (callbackMap.has(wrap)) {
     const { importModuleDynamically } = callbackMap.get(wrap);
     if (importModuleDynamically !== undefined) {
       return importModuleDynamically(
-        specifier, getModuleFromWrap(wrap) || wrap);
+        specifier, getModuleFromWrap(wrap) || wrap, assertions);
     }
   }
   throw new ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING();
@@ -69,6 +74,7 @@ async function initializeLoader() {
   const exports = await internalEsmLoader.import(
     customLoaders,
     pathToFileURL(cwd).href,
+    ObjectCreate(null),
   );
 
   // Hooks must then be added to external/public loader
diff --git a/lib/vm.js b/lib/vm.js
@@ -378,7 +378,7 @@ function compileFunction(code, params, options = {}) {
     const wrapped = importModuleDynamicallyWrap(importModuleDynamically);
     const func = result.function;
     callbackMap.set(result.cacheKey, {
-      importModuleDynamically: (s, _k) => wrapped(s, func),
+      importModuleDynamically: (s, _k, i) => wrapped(s, func, i),
     });
   }
 
diff --git a/src/module_wrap.cc b/src/module_wrap.cc
@@ -253,6 +253,21 @@ void ModuleWrap::New(const FunctionCallbackInfo<Value>& args) {
   args.GetReturnValue().Set(that);
 }
 
+static Local<Object> createImportAssertionContainer(Environment* env,
+  Isolate* isolate, Local<FixedArray> raw_assertions) {
+  Local<Object> assertions =
+        Object::New(isolate, v8::Null(env->isolate()), nullptr, nullptr, 0);
+  for (int i = 0; i < raw_assertions->Length(); i += 3) {
+      assertions
+          ->Set(env->context(),
+                raw_assertions->Get(env->context(), i).As<String>(),
+                raw_assertions->Get(env->context(), i + 1).As<Value>())
+          .ToChecked();
+  }
+
+  return assertions;
+}
+
 void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
   Environment* env = Environment::GetCurrent(args);
   Isolate* isolate = args.GetIsolate();
@@ -288,14 +303,7 @@ void ModuleWrap::Link(const FunctionCallbackInfo<Value>& args) {
 
     Local<FixedArray> raw_assertions = module_request->GetImportAssertions();
     Local<Object> assertions =
-        Object::New(isolate, v8::Null(env->isolate()), nullptr, nullptr, 0);
-    for (int i = 0; i < raw_assertions->Length(); i += 3) {
-      assertions
-          ->Set(env->context(),
-                raw_assertions->Get(env->context(), i).As<String>(),
-                raw_assertions->Get(env->context(), i + 1).As<Value>())
-          .ToChecked();
-    }
+      createImportAssertionContainer(env, isolate, raw_assertions);
 
     Local<Value> argv[] = {
         specifier,
@@ -602,9 +610,13 @@ static MaybeLocal<Promise> ImportModuleDynamically(
     UNREACHABLE();
   }
 
+  Local<Object> assertions =
+    createImportAssertionContainer(env, isolate, import_assertions);
+
   Local<Value> import_args[] = {
     object,
     Local<Value>(specifier),
+    assertions,
   };
 
   Local<Value> result;
diff --git a/test/parallel/test-vm-module-dynamic-import.js b/test/parallel/test-vm-module-dynamic-import.js
@@ -1,6 +1,6 @@
 'use strict';
 
-// Flags: --experimental-vm-modules
+// Flags: --experimental-vm-modules --harmony-import-assertions
 
 const common = require('../common');
 
@@ -56,6 +56,20 @@ async function test() {
     assert.strictEqual(foo.namespace, await globalThis.fooResult);
     delete globalThis.fooResult;
   }
+
+  {
+    const s = new Script('import("foo", { assert: { key: "value" } })', {
+      importModuleDynamically: common.mustCall((specifier, wrap, assertion) => {
+        assert.strictEqual(specifier, 'foo');
+        assert.strictEqual(wrap, s);
+        assert.deepStrictEqual(assertion, { __proto__: null, key: 'value' });
+        return foo;
+      }),
+    });
+
+    const result = s.runInThisContext();
+    assert.strictEqual(foo.namespace, await result);
+  }
 }
 
 async function testInvalid() {

Original file line number	Diff line number	Diff line change
`@@ -378,7 +378,7 @@ function compileFunction(code, params, options = {}) {`
`378`	`378`	`const wrapped = importModuleDynamicallyWrap(importModuleDynamically);`
`379`	`379`	`const func = result.function;`
`380`	`380`	`callbackMap.set(result.cacheKey, {`
`381`		`- importModuleDynamically: (s, _k) => wrapped(s, func),`
	`381`	`+ importModuleDynamically: (s, _k, i) => wrapped(s, func, i),`
`382`	`382`	`});`
`383`	`383`	`}`
`384`	`384`