From 12aff38044e4bc8ed39cf9139357ac914d68255c Mon Sep 17 00:00:00 2001 From: Gus Caplan Date: Fri, 25 Sep 2020 15:22:52 -0500 Subject: [PATCH] Revert "Revert "vm: add importModuleDynamically option to compileFunction"" This reverts commit 2d5d77306f6dff9110c1f77fefab25f973415770. See: https://github.com/nodejs/node/pull/32985 See: https://github.com/nodejs/node/pull/33364 See: https://github.com/nodejs/node/issues/33166 Fixes: https://github.com/nodejs/node/issues/31860 --- doc/api/vm.md | 15 +++++++++- lib/internal/modules/cjs/loader.js | 40 ++++++++------------------- lib/vm.js | 17 ++++++++++++ test/parallel/test-vm-module-basic.js | 19 ++++++++++++- tools/doc/type-parser.js | 1 + 5 files changed, 62 insertions(+), 30 deletions(-) diff --git a/doc/api/vm.md b/doc/api/vm.md index e6796b4c0fb702..f0c8df9602f85c 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -90,7 +90,7 @@ changes: This option is part of the experimental modules API, and should not be considered stable. * `specifier` {string} specifier passed to `import()` - * `module` {vm.Module} + * `script` {vm.Script} * 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. @@ -821,6 +821,9 @@ changes: - version: v14.3.0 pr-url: https://github.com/nodejs/node/pull/33364 description: Removal of `importModuleDynamically` due to compatibility issues + - version: REPLACEME + pr-url: REPLACEME + description: Added `importModuleDynamically` option again. --> * `code` {string} The body of the function to compile. @@ -843,6 +846,16 @@ changes: * `contextExtensions` {Object[]} An array containing a collection of context extensions (objects wrapping the current scope) to be applied while compiling. **Default:** `[]`. + * `importModuleDynamically` {Function} Called during evaluation of this module + when `import()` is called. If this option is not specified, calls to + `import()` will reject with [`ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING`][]. + This option is part of the experimental modules API, and should not be + considered stable. + * `specifier` {string} specifier passed to `import()` + * `function` {Function} + * 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. * Returns: {Function} Compiles the given code into the provided context (if no context is diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 34507ebdad38c2..6941d7877b8c4c 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -86,7 +86,6 @@ const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const policy = getOptionValue('--experimental-policy') ? require('internal/process/policy') : null; -const { compileFunction } = internalBinding('contextify'); // Whether any user-provided CJS modules had been loaded (executed). // Used for internal assertions. @@ -1000,40 +999,25 @@ function wrapSafe(filename, content, cjsModuleInstance) { }, }); } - let compiled; try { - compiled = compileFunction( - content, + return vm.compileFunction(content, [ + 'exports', + 'require', + 'module', + '__filename', + '__dirname', + ], { filename, - 0, - 0, - undefined, - false, - undefined, - [], - [ - 'exports', - 'require', - 'module', - '__filename', - '__dirname', - ] - ); + importModuleDynamically(specifier) { + const loader = asyncESM.ESMLoader; + return loader.import(specifier, normalizeReferrerURL(filename)); + }, + }); } catch (err) { if (process.mainModule === cjsModuleInstance) enrichCJSError(err); throw err; } - - const { callbackMap } = internalBinding('module_wrap'); - callbackMap.set(compiled.cacheKey, { - importModuleDynamically: async (specifier) => { - const loader = asyncESM.ESMLoader; - return loader.import(specifier, normalizeReferrerURL(filename)); - } - }); - - return compiled.function; } // Run the file contents in the correct scope or sandbox. Expose diff --git a/lib/vm.js b/lib/vm.js index 45a2edf0bb20b3..7a2dfc128d1b4e 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -325,6 +325,7 @@ function compileFunction(code, params, options = {}) { produceCachedData = false, parsingContext = undefined, contextExtensions = [], + importModuleDynamically, } = options; validateString(filename, 'options.filename'); @@ -372,6 +373,22 @@ function compileFunction(code, params, options = {}) { result.function.cachedData = result.cachedData; } + if (importModuleDynamically !== undefined) { + if (typeof importModuleDynamically !== 'function') { + throw new ERR_INVALID_ARG_TYPE('options.importModuleDynamically', + 'function', + importModuleDynamically); + } + const { importModuleDynamicallyWrap } = + require('internal/vm/module'); + const { callbackMap } = internalBinding('module_wrap'); + const wrapped = importModuleDynamicallyWrap(importModuleDynamically); + const func = result.function; + callbackMap.set(result.cacheKey, { + importModuleDynamically: (s, _k) => wrapped(s, func), + }); + } + return result.function; } diff --git a/test/parallel/test-vm-module-basic.js b/test/parallel/test-vm-module-basic.js index 0376473bf7123c..d162f1bebd744c 100644 --- a/test/parallel/test-vm-module-basic.js +++ b/test/parallel/test-vm-module-basic.js @@ -8,7 +8,8 @@ const { Module, SourceTextModule, SyntheticModule, - createContext + createContext, + compileFunction, } = require('vm'); const util = require('util'); @@ -160,3 +161,19 @@ const util = require('util'); name: 'TypeError' }); } + +// Test compileFunction importModuleDynamically +{ + const module = new SyntheticModule([], () => {}); + module.link(() => {}); + const f = compileFunction('return import("x")', [], { + importModuleDynamically(specifier, referrer) { + assert.strictEqual(specifier, 'x'); + assert.strictEqual(referrer, f); + return module; + }, + }); + f().then((ns) => { + assert.strictEqual(ns, module.namespace); + }); +} diff --git a/tools/doc/type-parser.js b/tools/doc/type-parser.js index a12df5714aa13f..34bed6b1ef84b0 100644 --- a/tools/doc/type-parser.js +++ b/tools/doc/type-parser.js @@ -168,6 +168,7 @@ const customTypesMap = { 'URLSearchParams': 'url.html#url_class_urlsearchparams', 'vm.Module': 'vm.html#vm_class_vm_module', + 'vm.Script': 'vm.html#vm_class_vm_script', 'vm.SourceTextModule': 'vm.html#vm_class_vm_sourcetextmodule', 'MessagePort': 'worker_threads.html#worker_threads_class_messageport',