From 2d5d77306f6dff9110c1f77fefab25f973415770 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 12 May 2020 11:18:34 +0200 Subject: [PATCH] Revert "vm: add importModuleDynamically option to compileFunction" This reverts commit 74c393dd93cc0e461e3796fbcc09545fcacdecaf. Fixes: https://github.com/nodejs/node/issues/33166 PR-URL: https://github.com/nodejs/node/pull/33364 Reviewed-By: Ruben Bridgewater Reviewed-By: Benjamin Gruenbaum Reviewed-By: Beth Griggs Reviewed-By: Colin Ihrig Reviewed-By: James M Snell Reviewed-By: Michael Dawson --- 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, 33 insertions(+), 59 deletions(-) diff --git a/doc/api/vm.md b/doc/api/vm.md index 982ec8412d5400..a28fc27a889714 100644 --- a/doc/api/vm.md +++ b/doc/api/vm.md @@ -88,7 +88,7 @@ changes: This option is part of the experimental modules API, and should not be considered stable. * `specifier` {string} specifier passed to `import()` - * `script` {vm.Script} + * `module` {vm.Module} * 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. @@ -807,6 +807,9 @@ changes: - v13.14.0 pr-url: https://github.com/nodejs/node/pull/32985 description: The `importModuleDynamically` option is now supported. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/33364 + description: Removal of `importModuleDynamically` due to compatibility issues --> * `code` {string} The body of the function to compile. @@ -829,16 +832,6 @@ 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 af28bc85e1e6b9..0c34e3425567ab 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -77,6 +77,7 @@ const preserveSymlinksMain = getOptionValue('--preserve-symlinks-main'); const manifest = getOptionValue('--experimental-policy') ? require('internal/process/policy').manifest : null; +const { compileFunction } = internalBinding('contextify'); // Whether any user-provided CJS modules had been loaded (executed). // Used for internal assertions. @@ -1110,25 +1111,40 @@ function wrapSafe(filename, content, cjsModuleInstance) { }, }); } + let compiled; try { - return vm.compileFunction(content, [ - 'exports', - 'require', - 'module', - '__filename', - '__dirname', - ], { + compiled = compileFunction( + content, filename, - importModuleDynamically(specifier) { - const loader = asyncESM.ESMLoader; - return loader.import(specifier, normalizeReferrerURL(filename)); - }, - }); + 0, + 0, + undefined, + false, + undefined, + [], + [ + 'exports', + 'require', + 'module', + '__filename', + '__dirname', + ] + ); } 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 fd81e9da8b0515..57d5d60cf5bb66 100644 --- a/lib/vm.js +++ b/lib/vm.js @@ -313,7 +313,6 @@ function compileFunction(code, params, options = {}) { produceCachedData = false, parsingContext = undefined, contextExtensions = [], - importModuleDynamically, } = options; validateString(filename, 'options.filename'); @@ -361,22 +360,6 @@ 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 c842ac01fb8d93..3e5e7759c1a9cb 100644 --- a/test/parallel/test-vm-module-basic.js +++ b/test/parallel/test-vm-module-basic.js @@ -8,8 +8,7 @@ const { Module, SourceTextModule, SyntheticModule, - createContext, - compileFunction, + createContext } = require('vm'); const util = require('util'); @@ -158,19 +157,3 @@ 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 1a687719338b12..08e9096a33e714 100644 --- a/tools/doc/type-parser.js +++ b/tools/doc/type-parser.js @@ -154,7 +154,6 @@ 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',