From 97aab2ea6a738aec8ec645213d67c1445b5f4dfd Mon Sep 17 00:00:00 2001 From: Antoine du Hamel Date: Wed, 29 Sep 2021 09:59:07 +0200 Subject: [PATCH] fixup! vm: add support for import assertions in dynamic imports --- doc/api/vm.md | 30 +++++++++++-------- lib/internal/process/esm_loader.js | 10 +++++-- src/module_wrap.cc | 11 +++++++ .../parallel/test-vm-module-dynamic-import.js | 16 +++++++++- 4 files changed, 52 insertions(+), 15 deletions(-) diff --git a/doc/api/vm.md b/doc/api/vm.md index 57cac0c12bbdb9..e4a95a35863337 100644 --- 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 index 0f04ca97751ef6..73385a85b4e106 100644 --- 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 index f45ee7627b0a9b..a9defba217626d 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -602,9 +602,20 @@ static MaybeLocal ImportModuleDynamically( UNREACHABLE(); } + Local 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::Cast(import_assertions->Get(env->context(), i)), + Local::Cast(import_assertions->Get(env->context(), i + 1))) + .ToChecked(); + } + Local import_args[] = { object, Local(specifier), + assertions, }; Local result; diff --git a/test/parallel/test-vm-module-dynamic-import.js b/test/parallel/test-vm-module-dynamic-import.js index 9d04cf96e6f979..2273497d27677c 100644 --- 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() {