fixup! vm: add support for import assertions in dynamic imports

nodejs · Sep 29, 2021 · 97aab2e · 97aab2e
1 parent f3b27e9
commit 97aab2e
Show file tree

Hide file tree

Showing 4 changed files with 52 additions and 15 deletions.
diff --git a/doc/api/vm.md b/doc/api/vm.md
@@ -56,7 +56,7 @@ added: v0.3.1
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/40249
-    description: Added support of import assertions to the
+    description: Added support for import assertions to the
                  `importModuleDynamically` parameter.
   - version: v10.6.0
     pr-url: https://github.com/nodejs/node/pull/20300
@@ -96,7 +96,8 @@ changes:
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAssertions` {Object} The `"assert"` value passed to the
-      `optionExpression` optional parameter.
+      `optionExpression` 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.
@@ -652,7 +653,7 @@ defined in the ECMAScript specification.
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/40249
-    description: Added support of import assertions to the
+    description: Added support for import assertions to the
                  `importModuleDynamically` parameter.
 -->
 
@@ -681,7 +682,8 @@ changes:
     * `specifier` {string} specifier passed to `import()`
     * `module` {vm.Module}
     * `importAssertions` {Object} The `"assert"` value passed to the
-      `optionExpression` optional parameter.
+      `optionExpression` 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.
@@ -869,7 +871,7 @@ added: v10.10.0
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/40249
-    description: Added support of import assertions to the
+    description: Added support for import assertions to the
                  `importModuleDynamically` parameter.
   - version: v15.9.0
     pr-url: https://github.com/nodejs/node/pull/35431
@@ -913,7 +915,8 @@ changes:
     * `specifier` {string} specifier passed to `import()`
     * `function` {Function}
     * `importAssertions` {Object} The `"assert"` value passed to the
-      `optionExpression` optional parameter.
+      `optionExpression` 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.
@@ -1091,7 +1094,7 @@ added: v0.3.1
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/40249
-    description: Added support of import assertions to the
+    description: Added support for import assertions to the
                  `importModuleDynamically` parameter.
   - version: v6.3.0
     pr-url: https://github.com/nodejs/node/pull/6635
@@ -1139,7 +1142,8 @@ changes:
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAssertions` {Object} The `"assert"` value passed to the
-      `optionExpression` optional parameter.
+      `optionExpression` 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.
@@ -1174,7 +1178,7 @@ added: v0.3.1
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/40249
-    description: Added support of import assertions to the
+    description: Added support for import assertions to the
                  `importModuleDynamically` parameter.
   - version: v14.6.0
     pr-url: https://github.com/nodejs/node/pull/34023
@@ -1243,7 +1247,8 @@ changes:
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAssertions` {Object} The `"assert"` value passed to the
-      `optionExpression` optional parameter.
+      `optionExpression` 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.
@@ -1282,7 +1287,7 @@ added: v0.3.1
 changes:
   - version: REPLACEME
     pr-url: https://github.com/nodejs/node/pull/40249
-    description: Added suppoort of import assertions to the
+    description: Added support for import assertions to the
                  `importModuleDynamically` parameter.
   - version: v6.3.0
     pr-url: https://github.com/nodejs/node/pull/6635
@@ -1328,7 +1333,8 @@ changes:
     * `specifier` {string} specifier passed to `import()`
     * `script` {vm.Script}
     * `importAssertions` {Object} The `"assert"` value passed to the
-      `optionExpression` optional parameter.
+      `optionExpression` 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.

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/src/module_wrap.cc b/src/module_wrap.cc
@@ -602,9 +602,20 @@ static MaybeLocal<Promise> ImportModuleDynamically(
     UNREACHABLE();
   }
 
+  Local<Object> assertions =
+    Object::New(isolate, v8::Null(env->isolate()), nullptr, nullptr, 0);
+  for (int i = 0; i < import_assertions->Length(); i += 2) {
+    assertions
+        ->Set(env->context(),
+              Local<String>::Cast(import_assertions->Get(env->context(), i)),
+              Local<Value>::Cast(import_assertions->Get(env->context(), i + 1)))
+        .ToChecked();
+  }
+
   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() {