From 366db481e47ac461f36b2450b8f2bb3537d9aa5e Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 12 Mar 2024 01:50:24 +0800 Subject: [PATCH 1/6] module: support require()ing synchronous ESM graphs This patch adds `require()` support for synchronous ESM graphs under the flag `--experimental-require-module` This is based on the the following design aspect of ESM: - The resolution can be synchronous (up to the host) - The evaluation of a synchronous graph (without top-level await) is also synchronous, and, by the time the module graph is instantiated (before evaluation starts), this is is already known. If `--experimental-require-module` is enabled, and the ECMAScript module being loaded by `require()` meets the following requirements: - Explicitly marked as an ES module with a `"type": "module"` field in the closest package.json or a `.mjs` extension. - Fully synchronous (contains no top-level `await`). `require()` will load the requested module as an ES Module, and return the module name space object. In this case it is similar to dynamic `import()` but is run synchronously and returns the name space object directly. ```mjs // point.mjs export function distance(a, b) { return (b.x - a.x) ** 2 + (b.y - a.y) ** 2; } class Point { constructor(x, y) { this.x = x; this.y = y; } } export default Point; ``` ```cjs const required = require('./point.mjs'); // [Module: null prototype] { // default: [class Point], // distance: [Function: distance] // } console.log(required); (async () => { const imported = await import('./point.mjs'); console.log(imported === required); // true })(); ``` If the module being `require()`'d contains top-level `await`, or the module graph it `import`s contains top-level `await`, [`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users should load the asynchronous module using `import()`. If `--experimental-print-required-tla` is enabled, instead of throwing `ERR_REQUIRE_ASYNC_MODULE` before evaluation, Node.js will evaluate the module, try to locate the top-level awaits, and print their location to help users fix them. PR-URL: https://github.com/nodejs/node/pull/51977 Reviewed-By: Chengzhong Wu Reviewed-By: Matteo Collina Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel Reviewed-By: Geoffrey Booth --- doc/api/cli.md | 27 +++ doc/api/errors.md | 16 ++ doc/api/esm.md | 8 +- doc/api/modules.md | 77 ++++++- doc/api/packages.md | 19 +- lib/internal/modules/cjs/loader.js | 114 +++++++++-- lib/internal/modules/esm/loader.js | 136 ++++++++++++- lib/internal/modules/esm/module_job.js | 67 +++++-- lib/internal/modules/esm/module_map.js | 4 +- lib/internal/modules/esm/translators.js | 32 ++- lib/internal/util/embedding.js | 2 +- src/module_wrap.cc | 189 ++++++++++++++++++ src/module_wrap.h | 8 + src/node_errors.h | 5 + src/node_options.cc | 11 + src/node_options.h | 2 + .../test-esm-cjs-load-error-note.mjs | 10 +- test/es-module/test-esm-loader-modulemap.js | 2 +- .../test-require-module-cached-tla.js | 14 ++ ...test-require-module-conditional-exports.js | 35 ++++ .../test-require-module-default-extension.js | 17 ++ .../test-require-module-dynamic-import-1.js | 32 +++ .../test-require-module-dynamic-import-2.js | 32 +++ test/es-module/test-require-module-errors.js | 48 +++++ .../es-module/test-require-module-implicit.js | 33 +++ test/es-module/test-require-module-preload.js | 72 +++++++ .../test-require-module-special-import.js | 11 + test/es-module/test-require-module-tla.js | 63 ++++++ test/es-module/test-require-module-twice.js | 21 ++ test/es-module/test-require-module.js | 62 ++++++ test/fixtures/es-modules/data-import.mjs | 2 + .../deprecated-folders-ignore/package.json | 1 - .../fixtures/es-modules/exports-both/load.cjs | 1 + .../exports-both/node_modules/dep/mod.cjs | 1 + .../exports-both/node_modules/dep/mod.mjs | 2 + .../node_modules/dep/package.json | 8 + .../exports-import-default/load.cjs | 1 + .../node_modules/dep/mod.js | 1 + .../node_modules/dep/mod.mjs | 2 + .../node_modules/dep/package.json | 8 + .../es-modules/exports-import-only/load.cjs | 2 + .../node_modules/dep/mod.js | 2 + .../node_modules/dep/package.json | 8 + .../es-modules/exports-require-only/load.cjs | 1 + .../node_modules/dep/mod.js | 1 + .../node_modules/dep/package.json | 7 + test/fixtures/es-modules/import-esm.mjs | 3 + test/fixtures/es-modules/imported-esm.mjs | 1 + test/fixtures/es-modules/network-import.mjs | 1 + .../package-default-extension/index.cjs | 1 + .../package-default-extension/index.mjs | 1 + test/fixtures/es-modules/reference-error.mjs | 3 + test/fixtures/es-modules/require-cjs.mjs | 5 + .../es-modules/require-reference-error.cjs | 2 + .../es-modules/require-syntax-error.cjs | 2 + .../es-modules/require-throw-error.cjs | 2 + test/fixtures/es-modules/required-cjs.js | 3 + .../es-modules/should-not-be-resolved.mjs | 1 + test/fixtures/es-modules/syntax-error.mjs | 1 + test/fixtures/es-modules/throw-error.mjs | 1 + test/fixtures/es-modules/tla/execution.mjs | 3 + .../es-modules/tla/require-execution.js | 1 + test/fixtures/es-modules/tla/resolved.mjs | 1 + 63 files changed, 1170 insertions(+), 79 deletions(-) create mode 100644 test/es-module/test-require-module-cached-tla.js create mode 100644 test/es-module/test-require-module-conditional-exports.js create mode 100644 test/es-module/test-require-module-default-extension.js create mode 100644 test/es-module/test-require-module-dynamic-import-1.js create mode 100644 test/es-module/test-require-module-dynamic-import-2.js create mode 100644 test/es-module/test-require-module-errors.js create mode 100644 test/es-module/test-require-module-implicit.js create mode 100644 test/es-module/test-require-module-preload.js create mode 100644 test/es-module/test-require-module-special-import.js create mode 100644 test/es-module/test-require-module-tla.js create mode 100644 test/es-module/test-require-module-twice.js create mode 100644 test/es-module/test-require-module.js create mode 100644 test/fixtures/es-modules/data-import.mjs create mode 100644 test/fixtures/es-modules/exports-both/load.cjs create mode 100644 test/fixtures/es-modules/exports-both/node_modules/dep/mod.cjs create mode 100644 test/fixtures/es-modules/exports-both/node_modules/dep/mod.mjs create mode 100644 test/fixtures/es-modules/exports-both/node_modules/dep/package.json create mode 100644 test/fixtures/es-modules/exports-import-default/load.cjs create mode 100644 test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.js create mode 100644 test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.mjs create mode 100644 test/fixtures/es-modules/exports-import-default/node_modules/dep/package.json create mode 100644 test/fixtures/es-modules/exports-import-only/load.cjs create mode 100644 test/fixtures/es-modules/exports-import-only/node_modules/dep/mod.js create mode 100644 test/fixtures/es-modules/exports-import-only/node_modules/dep/package.json create mode 100644 test/fixtures/es-modules/exports-require-only/load.cjs create mode 100644 test/fixtures/es-modules/exports-require-only/node_modules/dep/mod.js create mode 100644 test/fixtures/es-modules/exports-require-only/node_modules/dep/package.json create mode 100644 test/fixtures/es-modules/import-esm.mjs create mode 100644 test/fixtures/es-modules/imported-esm.mjs create mode 100644 test/fixtures/es-modules/network-import.mjs create mode 100644 test/fixtures/es-modules/package-default-extension/index.cjs create mode 100644 test/fixtures/es-modules/package-default-extension/index.mjs create mode 100644 test/fixtures/es-modules/reference-error.mjs create mode 100644 test/fixtures/es-modules/require-cjs.mjs create mode 100644 test/fixtures/es-modules/require-reference-error.cjs create mode 100644 test/fixtures/es-modules/require-syntax-error.cjs create mode 100644 test/fixtures/es-modules/require-throw-error.cjs create mode 100644 test/fixtures/es-modules/required-cjs.js create mode 100644 test/fixtures/es-modules/should-not-be-resolved.mjs create mode 100644 test/fixtures/es-modules/syntax-error.mjs create mode 100644 test/fixtures/es-modules/throw-error.mjs create mode 100644 test/fixtures/es-modules/tla/execution.mjs create mode 100644 test/fixtures/es-modules/tla/require-execution.js create mode 100644 test/fixtures/es-modules/tla/resolved.mjs diff --git a/doc/api/cli.md b/doc/api/cli.md index de3e00da7c7825..445cba786c347f 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -974,6 +974,18 @@ added: v11.8.0 Use the specified file as a security policy. +### `--experimental-require-module` + + + +> Stability: 1.1 - Active Developement + +Supports loading a synchronous ES module graph in `require()`. + +See [Loading ECMAScript modules using `require()`][]. + ### `--experimental-sea-config` + +This flag is only useful when `--experimental-require-module` is enabled. + +If the ES module being `require()`'d contains top-level await, this flag +allows Node.js to evaluate the module, try to locate the +top-level awaits, and print their location to help users find them. + ### `--prof` @@ -207,12 +251,24 @@ require(X) from module at path Y LOAD_AS_FILE(X) 1. If X is a file, load X as its file extension format. STOP -2. If X.js is a file, load X.js as JavaScript text. STOP -3. If X.json is a file, parse X.json to a JavaScript Object. STOP +2. If X.js is a file, + a. Find the closest package scope SCOPE to X. + b. If no scope was found, load X.js as a CommonJS module. STOP. + c. If the SCOPE/package.json contains "type" field, + 1. If the "type" field is "module", load X.js as an ECMAScript module. STOP. + 2. Else, load X.js as an CommonJS module. STOP. +3. If X.json is a file, load X.json to a JavaScript Object. STOP 4. If X.node is a file, load X.node as binary addon. STOP +5. If X.mjs is a file, and `--experimental-require-module` is enabled, + load X.mjs as an ECMAScript module. STOP LOAD_INDEX(X) -1. If X/index.js is a file, load X/index.js as JavaScript text. STOP +1. If X/index.js is a file + a. Find the closest package scope SCOPE to X. + b. If no scope was found, load X/index.js as a CommonJS module. STOP. + c. If the SCOPE/package.json contains "type" field, + 1. If the "type" field is "module", load X/index.js as an ECMAScript module. STOP. + 2. Else, load X/index.js as an CommonJS module. STOP. 2. If X/index.json is a file, parse X/index.json to a JavaScript object. STOP 3. If X/index.node is a file, load X/index.node as binary addon. STOP @@ -1097,6 +1153,7 @@ This section was moved to [GLOBAL_FOLDERS]: #loading-from-the-global-folders [`"main"`]: packages.md#main [`"type"`]: packages.md#type +[`ERR_REQUIRE_ASYNC_MODULE`]: errors.md#err_require_async_module [`ERR_REQUIRE_ESM`]: errors.md#err_require_esm [`ERR_UNSUPPORTED_DIR_IMPORT`]: errors.md#err_unsupported_dir_import [`MODULE_NOT_FOUND`]: errors.md#module_not_found diff --git a/doc/api/packages.md b/doc/api/packages.md index 8a5efdc89c4853..4e3414c66f7f7a 100644 --- a/doc/api/packages.md +++ b/doc/api/packages.md @@ -133,14 +133,15 @@ There is the CommonJS module loader: `process.dlopen()`. * It treats all files that lack `.json` or `.node` extensions as JavaScript text files. -* It cannot be used to load ECMAScript modules (although it is possible to - [load ECMASCript modules from CommonJS modules][]). When used to load a - JavaScript text file that is not an ECMAScript module, it loads it as a - CommonJS module. +* It can only be used to [load ECMASCript modules from CommonJS modules][] if + the module graph is synchronous (that contains no top-level `await`) when + `--experimental-require-module` is enabled. + When used to load a JavaScript text file that is not an ECMAScript module, + the file will be loaded as a CommonJS module. There is the ECMAScript module loader: -* It is asynchronous. +* It is asynchronous, unless it's being used to load modules for `require()`. * It is responsible for handling `import` statements and `import()` expressions. * It is not monkey patchable, can be customized using [loader hooks][]. * It does not support folders as modules, directory indexes (e.g. @@ -623,9 +624,9 @@ specific to least specific as conditions should be defined: * `"require"` - matches when the package is loaded via `require()`. The referenced file should be loadable with `require()` although the condition matches regardless of the module format of the target file. Expected - formats include CommonJS, JSON, and native addons but not ES modules as - `require()` doesn't support them. _Always mutually exclusive with - `"import"`._ + formats include CommonJS, JSON, native addons, and ES modules + if `--experimental-require-module` is enabled. _Always mutually + exclusive with `"import"`._ * `"default"` - the generic fallback that always matches. Can be a CommonJS or ES module file. _This condition should always come last._ @@ -1371,7 +1372,7 @@ This field defines [subpath imports][] for the current package. [entry points]: #package-entry-points [folders as modules]: modules.md#folders-as-modules [import maps]: https://github.com/WICG/import-maps -[load ECMASCript modules from CommonJS modules]: modules.md#the-mjs-extension +[load ECMASCript modules from CommonJS modules]: modules.md#loading-ecmascript-modules-using-require [loader hooks]: esm.md#loaders [packages folder mapping]: https://github.com/WICG/import-maps#packages-via-trailing-slashes [self-reference]: #self-referencing-a-package-using-its-name diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 7bbd59e16330b5..34bec05ff72455 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -60,10 +60,11 @@ const { StringPrototypeSlice, StringPrototypeSplit, StringPrototypeStartsWith, + Symbol, } = primordials; -// Map used to store CJS parsing data. -const cjsParseCache = new SafeWeakMap(); +// Map used to store CJS parsing data or for ESM loading. +const cjsSourceCache = new SafeWeakMap(); /** * Map of already-loaded CJS modules to use. */ @@ -72,12 +73,14 @@ const cjsExportsCache = new SafeWeakMap(); // Set first due to cycle with ESM loader functions. module.exports = { cjsExportsCache, - cjsParseCache, + cjsSourceCache, initializeCJS, Module, wrapSafe, }; +const kIsMainSymbol = Symbol('kIsMainSymbol'); + const { BuiltinModule } = require('internal/bootstrap/realm'); const { maybeCacheSourceMap, @@ -395,6 +398,11 @@ function initializeCJS() { // TODO(joyeecheung): deprecate this in favor of a proper hook? Module.runMain = require('internal/modules/run_main').executeUserEntryPoint; + + if (getOptionValue('--experimental-require-module')) { + emitExperimentalWarning('Support for loading ES Module in require()'); + Module._extensions['.mjs'] = loadESMFromCJS; + } } // Given a module name, and a list of paths to test, returns the first @@ -601,6 +609,19 @@ function resolveExports(nmPath, request) { } } +// We don't cache this in case user extends the extensions. +function getDefaultExtensions() { + const extensions = ObjectKeys(Module._extensions); + if (!getOptionValue('--experimental-require-module')) { + return extensions; + } + // If the .mjs extension is added by --experimental-require-module, + // remove it from the supported default extensions to maintain + // compatibility. + // TODO(joyeecheung): allow both .mjs and .cjs? + return ArrayPrototypeFilter(extensions, (ext) => ext !== '.mjs' || Module._extensions['.mjs'] !== loadESMFromCJS); +} + /** * Get the absolute path to a module. * @param {string} request Relative or absolute file path @@ -702,7 +723,7 @@ Module._findPath = function(request, paths, isMain) { if (!filename) { // Try it with each of the extensions if (exts === undefined) { - exts = ObjectKeys(Module._extensions); + exts = getDefaultExtensions(); } filename = tryExtensions(basePath, exts, isMain); } @@ -711,7 +732,7 @@ Module._findPath = function(request, paths, isMain) { if (!filename && rc === 1) { // Directory. // try it with each of the extensions at "index" if (exts === undefined) { - exts = ObjectKeys(Module._extensions); + exts = getDefaultExtensions(); } filename = tryPackage(basePath, exts, isMain, request); } @@ -988,7 +1009,7 @@ Module._load = function(request, parent, isMain) { if (cachedModule !== undefined) { updateChildren(parent, cachedModule, true); if (!cachedModule.loaded) { - const parseCachedModule = cjsParseCache.get(cachedModule); + const parseCachedModule = cjsSourceCache.get(cachedModule); if (!parseCachedModule || parseCachedModule.loaded) { return getExportsForCircularRequire(cachedModule); } @@ -1010,6 +1031,9 @@ Module._load = function(request, parent, isMain) { setOwnProperty(process, 'mainModule', module); setOwnProperty(module.require, 'main', process.mainModule); module.id = '.'; + module[kIsMainSymbol] = true; + } else { + module[kIsMainSymbol] = false; } reportModuleToWatchMode(filename); @@ -1245,6 +1269,20 @@ let resolvedArgv; let hasPausedEntry = false; /** @type {import('vm').Script} */ +/** + * Resolve and evaluate it synchronously as ESM if it's ESM. + * @param {Module} mod CJS module instance + * @param {string} filename Absolute path of the file. + */ +function loadESMFromCJS(mod, filename) { + const source = getMaybeCachedSource(mod, filename); + const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); + const isMain = mod[kIsMainSymbol]; + // TODO(joyeecheung): we may want to invent optional special handling for default exports here. + // For now, it's good enough to be identical to what `import()` returns. + mod.exports = cascadedLoader.importSyncForRequire(filename, source, isMain); +} + /** * Wraps the given content in a script and runs it in a new context. * @param {string} filename The name of the file being loaded @@ -1270,11 +1308,16 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) { ); // Cache the source map for the module if present. - if (script.sourceMapURL) { - maybeCacheSourceMap(filename, content, this, false, undefined, script.sourceMapURL); + const { sourceMapURL } = script; + if (sourceMapURL) { + maybeCacheSourceMap(filename, content, this, false, undefined, sourceMapURL); } - return runScriptInThisContext(script, true, false); + return { + __proto__: null, + function: runScriptInThisContext(script, true, false), + sourceMapURL, + }; } try { @@ -1292,7 +1335,7 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) { maybeCacheSourceMap(filename, content, this, false, undefined, result.sourceMapURL); } - return result.function; + return result; } catch (err) { if (process.mainModule === cjsModuleInstance) { const { enrichCJSError } = require('internal/modules/esm/translators'); @@ -1307,8 +1350,9 @@ function wrapSafe(filename, content, cjsModuleInstance, codeCache) { * `exports`) to the file. Returns exception, if any. * @param {string} content The source code of the module * @param {string} filename The file path of the module + * @param {boolean} loadAsESM Whether it's known to be ESM via .mjs or "type" in package.json. */ -Module.prototype._compile = function(content, filename) { +Module.prototype._compile = function(content, filename, loadAsESM = false) { let moduleURL; let redirects; const manifest = policy()?.manifest; @@ -1318,8 +1362,20 @@ Module.prototype._compile = function(content, filename) { manifest.assertIntegrity(moduleURL, content); } - const compiledWrapper = wrapSafe(filename, content, this); + // TODO(joyeecheung): when the module is the entry point, consider allowing TLA. + // Only modules being require()'d really need to avoid TLA. + if (loadAsESM) { + // Pass the source into the .mjs extension handler indirectly through the cache. + cjsSourceCache.set(this, { source: content }); + loadESMFromCJS(this, filename); + return; + } + const { function: compiledWrapper } = wrapSafe(filename, content, this); + + // TODO(joyeecheung): the detection below is unnecessarily complex. Using the + // kIsMainSymbol, or a kBreakOnStartSymbol that gets passed from + // higher level instead of doing hacky detection here. let inspectorWrapper = null; if (getOptionValue('--inspect-brk') && process._eval == null) { if (!resolvedArgv) { @@ -1363,24 +1419,43 @@ Module.prototype._compile = function(content, filename) { }; /** - * Native handler for `.js` files. - * @param {Module} module The module to compile - * @param {string} filename The file path of the module + * Get the source code of a module, using cached ones if it's cached. + * @param {Module} mod Module instance whose source is potentially already cached. + * @param {string} filename Absolute path to the file of the module. + * @returns {string} */ -Module._extensions['.js'] = function(module, filename) { - // If already analyzed the source, then it will be cached. - const cached = cjsParseCache.get(module); +function getMaybeCachedSource(mod, filename) { + const cached = cjsSourceCache.get(mod); let content; if (cached?.source) { content = cached.source; cached.source = undefined; } else { + // TODO(joyeecheung): we can read a buffer instead to speed up + // compilation. content = fs.readFileSync(filename, 'utf8'); } + return content; +} + +/** + * Built-in handler for `.js` files. + * @param {Module} module The module to compile + * @param {string} filename The file path of the module + */ +Module._extensions['.js'] = function(module, filename) { + // If already analyzed the source, then it will be cached. + const content = getMaybeCachedSource(module, filename); + if (StringPrototypeEndsWith(filename, '.js')) { const pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null }; // Function require shouldn't be used in ES modules. if (pkg.data?.type === 'module') { + if (getOptionValue('--experimental-require-module')) { + module._compile(content, filename, true); + return; + } + // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed. const parent = moduleParentCache.get(module); const parentPath = parent?.filename; @@ -1413,7 +1488,8 @@ Module._extensions['.js'] = function(module, filename) { throw err; } } - module._compile(content, filename); + + module._compile(content, filename, false); }; /** diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 7ab5078ffc9307..bdcd6028dacb39 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -15,16 +15,25 @@ const { hardenRegExp, } = primordials; +const assert = require('internal/assert'); const { ERR_REQUIRE_ESM, + ERR_NETWORK_IMPORT_DISALLOWED, ERR_UNKNOWN_MODULE_FORMAT, } = require('internal/errors').codes; const { getOptionValue } = require('internal/options'); -const { isURL } = require('internal/url'); -const { emitExperimentalWarning } = require('internal/util'); +const { isURL, pathToFileURL, URL } = require('internal/url'); +const { emitExperimentalWarning, kEmptyObject } = require('internal/util'); const { + registerModule, getDefaultConditions, } = require('internal/modules/esm/utils'); +const { kImplicitAssertType } = require('internal/modules/esm/assert'); +const { + maybeCacheSourceMap, +} = require('internal/source_map/source_map_cache'); +const { canParse } = internalBinding('url'); +const { ModuleWrap } = internalBinding('module_wrap'); let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; /** @@ -184,8 +193,6 @@ class ModuleLoader { async eval(source, url) { const evalInstance = (url) => { - const { ModuleWrap } = internalBinding('module_wrap'); - const { registerModule } = require('internal/modules/esm/utils'); const module = new ModuleWrap(url, undefined, source, 0, 0); registerModule(module, { __proto__: null, @@ -197,7 +204,7 @@ class ModuleLoader { return module; }; - const ModuleJob = require('internal/modules/esm/module_job'); + const { ModuleJob } = require('internal/modules/esm/module_job'); const job = new ModuleJob( this, url, undefined, evalInstance, false, false); this.loadCache.set(url, undefined, job); @@ -250,6 +257,118 @@ class ModuleLoader { return job; } + /** + * This constructs (creates, instantiates and evaluates) a module graph that + * is require()'d. + * @param {string} filename Resolved filename of the module being require()'d + * @param {string} source Source code. TODO(joyeecheung): pass the raw buffer. + * @param {string} isMain Whether this module is a main module. + * @returns {ModuleNamespaceObject} + */ + importSyncForRequire(filename, source, isMain) { + const url = pathToFileURL(filename).href; + let job = this.loadCache.get(url, kImplicitAssertType); + // This module is already loaded, check whether it's synchronous and return the + // namespace. + if (job !== undefined) { + return job.module.getNamespaceSync(); + } + + // TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the + // cache here, or use a carrier object to carry the compiled module script + // into the constructor to ensure cache hit. + const wrap = new ModuleWrap(url, undefined, source, 0, 0); + // Cache the source map for the module if present. + if (wrap.sourceMapURL) { + maybeCacheSourceMap(url, source, null, false, undefined, wrap.sourceMapURL); + } + const { registerModule } = require('internal/modules/esm/utils'); + // TODO(joyeecheung): refactor so that the default options are shared across + // the built-in loaders. + registerModule(wrap, { + __proto__: null, + initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), + importModuleDynamically: (specifier, wrap, importAttributes) => { + return this.import(specifier, url, importAttributes); + }, + }); + + const inspectBrk = (isMain && getOptionValue('--inspect-brk')); + + const { ModuleJobSync } = require('internal/modules/esm/module_job'); + job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk); + this.loadCache.set(url, kImplicitAssertType, job); + return job.runSync().namespace; + } + + /** + * Resolve individual module requests and create or get the cached ModuleWraps for + * each of them. This is only used to create a module graph being require()'d. + * @param {string} specifier Specifier of the the imported module. + * @param {string} parentURL Where the import comes from. + * @param {object} importAttributes import attributes from the import statement. + * @returns {ModuleWrap} + */ + getModuleWrapForRequire(specifier, parentURL, importAttributes) { + assert(getOptionValue('--experimental-require-module')); + + if (canParse(specifier)) { + const protocol = new URL(specifier).protocol; + if (protocol === 'https:' || protocol === 'http:') { + throw new ERR_NETWORK_IMPORT_DISALLOWED(specifier, parentURL, + 'ES modules cannot be loaded by require() from the network'); + } + assert(protocol === 'file:' || protocol === 'node:' || protocol === 'data:'); + } + + const requestKey = this.#resolveCache.serializeKey(specifier, importAttributes); + let resolveResult = this.#resolveCache.get(requestKey, parentURL); + if (resolveResult == null) { + resolveResult = this.defaultResolve(specifier, parentURL, importAttributes); + this.#resolveCache.set(requestKey, parentURL, resolveResult); + } + + const { url, format } = resolveResult; + const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes; + let job = this.loadCache.get(url, resolvedImportAttributes.type); + if (job !== undefined) { + // This module is previously imported before. We will return the module now and check + // asynchronicity of the entire graph later, after the graph is instantiated. + return job.module; + } + + defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; + const loadResult = defaultLoadSync(url, { format, importAttributes }); + const { responseURL, source } = loadResult; + let { format: finalFormat } = loadResult; + this.validateLoadResult(url, finalFormat); + if (finalFormat === 'commonjs') { + finalFormat = 'commonjs-sync'; + } else if (finalFormat === 'wasm') { + assert.fail('WASM is currently unsupported by require(esm)'); + } + + const translator = getTranslators().get(finalFormat); + if (!translator) { + throw new ERR_UNKNOWN_MODULE_FORMAT(finalFormat, responseURL); + } + + const isMain = (parentURL === undefined); + const wrap = FunctionPrototypeCall(translator, this, responseURL, source, isMain); + assert(wrap instanceof ModuleWrap); // No asynchronous translators should be called. + + if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { + process.send({ 'watch:import': [url] }); + } + + const inspectBrk = (isMain && getOptionValue('--inspect-brk')); + const { ModuleJobSync } = require('internal/modules/esm/module_job'); + job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk); + + this.loadCache.set(url, importAttributes.type, job); + return job.module; + } + /** * Create and cache an object representing a loaded module. * @param {string} url The absolute URL that was resolved for this module @@ -277,8 +396,9 @@ class ModuleLoader { (url, isMain) => callTranslator(this.loadSync(url, context), isMain) : async (url, isMain) => callTranslator(await this.load(url, context), isMain); + const isMain = parentURL === undefined; const inspectBrk = ( - parentURL === undefined && + isMain && getOptionValue('--inspect-brk') ); @@ -286,13 +406,13 @@ class ModuleLoader { process.send({ 'watch:import': [url] }); } - const ModuleJob = require('internal/modules/esm/module_job'); + const { ModuleJob } = require('internal/modules/esm/module_job'); const job = new ModuleJob( this, url, importAttributes, moduleProvider, - parentURL === undefined, + isMain, inspectBrk, sync, ); diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index ba62fd4361a9a6..a9e0f72579b6f8 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -8,9 +8,9 @@ const { ObjectSetPrototypeOf, PromiseResolve, PromisePrototypeThen, - ReflectApply, RegExpPrototypeExec, RegExpPrototypeSymbolReplace, + ReflectApply, SafePromiseAllReturnArrayLike, SafePromiseAllReturnVoid, SafeSet, @@ -19,7 +19,7 @@ const { StringPrototypeStartsWith, } = primordials; -const { ModuleWrap } = internalBinding('module_wrap'); +const { ModuleWrap, kEvaluated } = internalBinding('module_wrap'); const { decorateErrorStack, kEmptyObject } = require('internal/util'); const { @@ -47,24 +47,29 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) => (globalLike) => errorMessage === `${globalLike} is not defined`, ); -/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of - * its dependencies, over time. */ -class ModuleJob { - // `loader` is the Loader instance used for loading dependencies. - // `moduleProvider` is a function - constructor(loader, url, importAttributes = { __proto__: null }, - moduleProvider, isMain, inspectBrk, sync = false) { +class ModuleJobBase { + constructor(loader, url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) { this.loader = loader; this.importAttributes = importAttributes; this.isMain = isMain; this.inspectBrk = inspectBrk; this.url = url; + this.module = moduleWrapMaybePromise; + } +} - this.module = undefined; +/* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of + * its dependencies, over time. */ +class ModuleJob extends ModuleJobBase { + // `loader` is the Loader instance used for loading dependencies. + constructor(loader, url, importAttributes = { __proto__: null }, + moduleProvider, isMain, inspectBrk, sync = false) { + const modulePromise = ReflectApply(moduleProvider, loader, [url, isMain]); + super(loader, url, importAttributes, modulePromise, isMain, inspectBrk); // Expose the promise to the ModuleWrap directly for linking below. // `this.module` is also filled in below. - this.modulePromise = ReflectApply(moduleProvider, loader, [url, isMain]); + this.modulePromise = modulePromise; if (sync) { this.module = this.modulePromise; @@ -247,5 +252,41 @@ class ModuleJob { return { __proto__: null, module: this.module }; } } -ObjectSetPrototypeOf(ModuleJob.prototype, null); -module.exports = ModuleJob; + +// This is a fully synchronous job and does not spawn additional threads in any way. +// All the steps are ensured to be synchronous and it throws on instantiating +// an asynchronous graph. +class ModuleJobSync extends ModuleJobBase { + constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) { + super(loader, url, importAttributes, moduleWrap, isMain, inspectBrk, true); + assert(this.module instanceof ModuleWrap); + const moduleRequests = this.module.getModuleRequestsSync(); + for (let i = 0; i < moduleRequests.length; ++i) { + const { 0: specifier, 1: attributes } = moduleRequests[i]; + const wrap = this.loader.getModuleWrapForRequire(specifier, url, attributes); + const isLast = (i === moduleRequests.length - 1); + // TODO(joyeecheung): make the resolution callback deal with both promisified + // an raw module wraps, then we don't need to wrap it with a promise here. + this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(wrap), isLast); + } + } + + async run() { + const status = this.module.getStatus(); + assert(status === kEvaluated, + `A require()-d module that is imported again must be evaluated. Status = ${status}`); + return { __proto__: null, module: this.module }; + } + + runSync() { + this.module.instantiateSync(); + setHasStartedUserESMExecution(); + const namespace = this.module.evaluateSync(); + return { __proto__: null, module: this.module, namespace }; + } +} + +ObjectSetPrototypeOf(ModuleJobBase.prototype, null); +module.exports = { + ModuleJob, ModuleJobSync, ModuleJobBase, +}; diff --git a/lib/internal/modules/esm/module_map.js b/lib/internal/modules/esm/module_map.js index eab00386c413a5..ab1171eaa47b02 100644 --- a/lib/internal/modules/esm/module_map.js +++ b/lib/internal/modules/esm/module_map.js @@ -97,8 +97,8 @@ class LoadCache extends SafeMap { validateString(url, 'url'); validateString(type, 'type'); - const ModuleJob = require('internal/modules/esm/module_job'); - if (job instanceof ModuleJob !== true && + const { ModuleJobBase } = require('internal/modules/esm/module_job'); + if (job instanceof ModuleJobBase !== true && typeof job !== 'function') { throw new ERR_INVALID_ARG_TYPE('job', 'ModuleJob', job); } diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 6772bbffd989d2..7312bd0b09f41a 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -44,7 +44,7 @@ const { } = require('internal/modules/helpers'); const { Module: CJSModule, - cjsParseCache, + cjsSourceCache, cjsExportsCache, } = require('internal/modules/cjs/loader'); const { fileURLToPath, pathToFileURL, URL } = require('internal/url'); @@ -81,7 +81,7 @@ let cjsParse; */ async function initCJSParse() { if (typeof WebAssembly === 'undefined') { - cjsParse = require('internal/deps/cjs-module-lexer/lexer').parse; + initCJSParseSync(); } else { const { parse, init } = require('internal/deps/cjs-module-lexer/dist/lexer'); @@ -89,11 +89,19 @@ async function initCJSParse() { await init(); cjsParse = parse; } catch { - cjsParse = require('internal/deps/cjs-module-lexer/lexer').parse; + initCJSParseSync(); } } } +function initCJSParseSync() { + // TODO(joyeecheung): implement a binding that directly compiles using + // v8::WasmModuleObject::Compile() synchronously. + if (cjsParse === undefined) { + cjsParse = require('internal/deps/cjs-module-lexer/lexer').parse; + } +} + const translators = new SafeMap(); exports.translators = translators; exports.enrichCJSError = enrichCJSError; @@ -162,7 +170,7 @@ async function importModuleDynamically(specifier, { url }, attributes) { } // Strategy for loading a standard JavaScript module. -translators.set('module', async function moduleStrategy(url, source, isMain) { +translators.set('module', function moduleStrategy(url, source, isMain) { assertBufferSource(source, true, 'load'); source = stringify(source); debug(`Translating StandardModule ${url}`); @@ -323,6 +331,16 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) { } +translators.set('commonjs-sync', function requireCommonJS(url, source, isMain) { + initCJSParseSync(); + assert(!isMain); // This is only used by imported CJS modules. + + return createCJSModuleWrap(url, source, isMain, (module, source, url, filename) => { + assert(module === CJSModule._cache[filename]); + CJSModule._load(filename, null, false); + }); +}); + // Handle CommonJS modules referenced by `require` calls. // This translator function must be sync, as `require` is sync. translators.set('require-commonjs', (url, source, isMain) => { @@ -371,7 +389,7 @@ function cjsPreparseModuleExports(filename, source) { // TODO: Do we want to keep hitting the user mutable CJS loader here? let module = CJSModule._cache[filename]; if (module) { - const cached = cjsParseCache.get(module); + const cached = cjsSourceCache.get(module); if (cached) { return { module, exportNames: cached.exportNames }; } @@ -395,7 +413,7 @@ function cjsPreparseModuleExports(filename, source) { const exportNames = new SafeSet(new SafeArrayIterator(exports)); // Set first for cycles. - cjsParseCache.set(module, { source, exportNames }); + cjsSourceCache.set(module, { source, exportNames }); if (reexports.length) { module.filename = filename; @@ -522,6 +540,8 @@ translators.set('wasm', async function(url, source) { let compiled; try { + // TODO(joyeecheung): implement a binding that directly compiles using + // v8::WasmModuleObject::Compile() synchronously. compiled = await WebAssembly.compile(source); } catch (err) { err.message = errPath(url) + ': ' + err.message; diff --git a/lib/internal/util/embedding.js b/lib/internal/util/embedding.js index db9162369aae0d..7e4cd565492843 100644 --- a/lib/internal/util/embedding.js +++ b/lib/internal/util/embedding.js @@ -16,7 +16,7 @@ const { getCodePath, isSea } = internalBinding('sea'); function embedderRunCjs(contents) { const filename = process.execPath; - const compiledWrapper = wrapSafe( + const { function: compiledWrapper } = wrapSafe( isSea() ? getCodePath() : filename, contents); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 501e4d7b7ea180..c2e2aa37ddefc5 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -279,6 +279,61 @@ static Local createImportAttributesContainer( return attributes; } +void ModuleWrap::GetModuleRequestsSync( + const FunctionCallbackInfo& args) { + Realm* realm = Realm::GetCurrent(args); + Isolate* isolate = args.GetIsolate(); + + Local that = args.This(); + + ModuleWrap* obj; + ASSIGN_OR_RETURN_UNWRAP(&obj, that); + + CHECK(!obj->linked_); + + Local module = obj->module_.Get(isolate); + Local module_requests = module->GetModuleRequests(); + const int module_requests_length = module_requests->Length(); + + std::vector> requests; + requests.reserve(module_requests_length); + // call the dependency resolve callbacks + for (int i = 0; i < module_requests_length; i++) { + Local module_request = + module_requests->Get(realm->context(), i).As(); + Local raw_attributes = module_request->GetImportAssertions(); + std::vector> request = { + module_request->GetSpecifier(), + createImportAttributesContainer(realm, isolate, raw_attributes, 3), + }; + requests.push_back(Array::New(isolate, request.data(), request.size())); + } + + args.GetReturnValue().Set( + Array::New(isolate, requests.data(), requests.size())); +} + +void ModuleWrap::CacheResolvedWrapsSync( + const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + + CHECK_EQ(args.Length(), 3); + CHECK(args[0]->IsString()); + CHECK(args[1]->IsPromise()); + CHECK(args[2]->IsBoolean()); + + ModuleWrap* dependent; + ASSIGN_OR_RETURN_UNWRAP(&dependent, args.This()); + + Utf8Value specifier(isolate, args[0]); + dependent->resolve_cache_[specifier.ToString()].Reset(isolate, + args[1].As()); + + if (args[2].As()->Value()) { + dependent->linked_ = true; + } +} + void ModuleWrap::Link(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); Isolate* isolate = args.GetIsolate(); @@ -444,6 +499,129 @@ void ModuleWrap::Evaluate(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(result.ToLocalChecked()); } +void ModuleWrap::InstantiateSync(const FunctionCallbackInfo& args) { + Realm* realm = Realm::GetCurrent(args); + Isolate* isolate = args.GetIsolate(); + ModuleWrap* obj; + ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); + Local context = obj->context(); + Local module = obj->module_.Get(isolate); + Environment* env = realm->env(); + + { + TryCatchScope try_catch(env); + USE(module->InstantiateModule(context, ResolveModuleCallback)); + + // clear resolve cache on instantiate + obj->resolve_cache_.clear(); + + if (try_catch.HasCaught() && !try_catch.HasTerminated()) { + CHECK(!try_catch.Message().IsEmpty()); + CHECK(!try_catch.Exception().IsEmpty()); + AppendExceptionLine(env, + try_catch.Exception(), + try_catch.Message(), + ErrorHandlingMode::MODULE_ERROR); + try_catch.ReThrow(); + return; + } + } + + // If --experimental-print-required-tla is true, proceeds to evaluation even + // if it's async because we want to search for the TLA and help users locate + // them. + if (module->IsGraphAsync() && !env->options()->print_required_tla) { + THROW_ERR_REQUIRE_ASYNC_MODULE(env); + return; + } +} + +void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { + Realm* realm = Realm::GetCurrent(args); + Isolate* isolate = args.GetIsolate(); + ModuleWrap* obj; + ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); + Local context = obj->context(); + Local module = obj->module_.Get(isolate); + Environment* env = realm->env(); + + Local result; + { + TryCatchScope try_catch(env); + if (!module->Evaluate(context).ToLocal(&result)) { + if (try_catch.HasCaught()) { + if (!try_catch.HasTerminated()) { + try_catch.ReThrow(); + } + return; + } + } + } + + CHECK(result->IsPromise()); + Local promise = result.As(); + if (promise->State() == Promise::PromiseState::kRejected) { + Local exception = promise->Result(); + Local message = + v8::Exception::CreateMessage(isolate, exception); + AppendExceptionLine( + env, exception, message, ErrorHandlingMode::MODULE_ERROR); + isolate->ThrowException(exception); + return; + } + + if (module->IsGraphAsync()) { + CHECK(env->options()->print_required_tla); + auto stalled = module->GetStalledTopLevelAwaitMessage(isolate); + if (stalled.size() != 0) { + for (auto pair : stalled) { + Local message = std::get<1>(pair); + + std::string reason = "Error: unexpected top-level await at "; + std::string info = + FormatErrorMessage(isolate, context, "", message, true); + reason += info; + FPrintF(stderr, "%s\n", reason); + } + } + THROW_ERR_REQUIRE_ASYNC_MODULE(env); + return; + } + + CHECK_EQ(promise->State(), Promise::PromiseState::kFulfilled); + + args.GetReturnValue().Set(module->GetModuleNamespace()); +} + +void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo& args) { + Realm* realm = Realm::GetCurrent(args); + Isolate* isolate = args.GetIsolate(); + ModuleWrap* obj; + ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); + Local module = obj->module_.Get(isolate); + + switch (module->GetStatus()) { + case v8::Module::Status::kUninstantiated: + case v8::Module::Status::kInstantiating: + return realm->env()->ThrowError( + "cannot get namespace, module has not been instantiated"); + case v8::Module::Status::kEvaluating: + return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env()); + case v8::Module::Status::kInstantiated: + case v8::Module::Status::kEvaluated: + case v8::Module::Status::kErrored: + break; + default: + UNREACHABLE(); + } + + if (module->IsGraphAsync()) { + return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env()); + } + Local result = module->GetModuleNamespace(); + args.GetReturnValue().Set(result); +} + void ModuleWrap::GetNamespace(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); Isolate* isolate = args.GetIsolate(); @@ -776,6 +954,12 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data, ModuleWrap::kInternalFieldCount); SetProtoMethod(isolate, tpl, "link", Link); + SetProtoMethod(isolate, tpl, "getModuleRequestsSync", GetModuleRequestsSync); + SetProtoMethod( + isolate, tpl, "cacheResolvedWrapsSync", CacheResolvedWrapsSync); + SetProtoMethod(isolate, tpl, "instantiateSync", InstantiateSync); + SetProtoMethod(isolate, tpl, "evaluateSync", EvaluateSync); + SetProtoMethod(isolate, tpl, "getNamespaceSync", GetNamespaceSync); SetProtoMethod(isolate, tpl, "instantiate", Instantiate); SetProtoMethod(isolate, tpl, "evaluate", Evaluate); SetProtoMethod(isolate, tpl, "setExport", SetSyntheticExport); @@ -827,6 +1011,11 @@ void ModuleWrap::RegisterExternalReferences( registry->Register(New); registry->Register(Link); + registry->Register(GetModuleRequestsSync); + registry->Register(CacheResolvedWrapsSync); + registry->Register(InstantiateSync); + registry->Register(EvaluateSync); + registry->Register(GetNamespaceSync); registry->Register(Instantiate); registry->Register(Evaluate); registry->Register(SetSyntheticExport); diff --git a/src/module_wrap.h b/src/module_wrap.h index e17048357feca2..45a338b38e01c8 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -78,6 +78,14 @@ class ModuleWrap : public BaseObject { ~ModuleWrap() override; static void New(const v8::FunctionCallbackInfo& args); + static void GetModuleRequestsSync( + const v8::FunctionCallbackInfo& args); + static void CacheResolvedWrapsSync( + const v8::FunctionCallbackInfo& args); + static void InstantiateSync(const v8::FunctionCallbackInfo& args); + static void EvaluateSync(const v8::FunctionCallbackInfo& args); + static void GetNamespaceSync(const v8::FunctionCallbackInfo& args); + static void Link(const v8::FunctionCallbackInfo& args); static void Instantiate(const v8::FunctionCallbackInfo& args); static void Evaluate(const v8::FunctionCallbackInfo& args); diff --git a/src/node_errors.h b/src/node_errors.h index 30f66a7648bff4..ad40141ca92c5a 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -92,6 +92,7 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details); V(ERR_MODULE_NOT_FOUND, Error) \ V(ERR_NON_CONTEXT_AWARE_DISABLED, Error) \ V(ERR_OUT_OF_RANGE, RangeError) \ + V(ERR_REQUIRE_ASYNC_MODULE, Error) \ V(ERR_SCRIPT_EXECUTION_INTERRUPTED, Error) \ V(ERR_SCRIPT_EXECUTION_TIMEOUT, Error) \ V(ERR_STRING_TOO_LONG, Error) \ @@ -192,6 +193,10 @@ ERRORS_WITH_CODE(V) "creating Workers") \ V(ERR_NON_CONTEXT_AWARE_DISABLED, \ "Loading non context-aware native addons has been disabled") \ + V(ERR_REQUIRE_ASYNC_MODULE, \ + "require() cannot be used on an ESM graph with top-level await. Use " \ + "import() instead. To see where the top-level await comes from, use " \ + "--experimental-print-required-tla.") \ V(ERR_SCRIPT_EXECUTION_INTERRUPTED, \ "Script execution was interrupted by `SIGINT`") \ V(ERR_TLS_PSK_SET_IDENTIY_HINT_FAILED, "Failed to set PSK identity hint") \ diff --git a/src/node_options.cc b/src/node_options.cc index 1ba0bfcd9b3096..4b3017546525dc 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -363,6 +363,17 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { "ES module syntax, try again to evaluate them as ES modules", &EnvironmentOptions::detect_module, kAllowedInEnvvar); + AddOption("--experimental-print-required-tla", + "Print pending top-level await. If --experimental-require-module " + "is true, evaluate asynchronous graphs loaded by `require()` but " + "do not run the microtasks, in order to to find and print " + "top-level await in the graph", + &EnvironmentOptions::print_required_tla, + kAllowedInEnvvar); + AddOption("--experimental-require-module", + "Allow loading explicit ES Modules in require().", + &EnvironmentOptions::require_module, + kAllowedInEnvvar); AddOption("--diagnostic-dir", "set dir for all output files" " (default: current working directory)", diff --git a/src/node_options.h b/src/node_options.h index 1357e5b42869e8..9e12730f878190 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -111,6 +111,8 @@ class EnvironmentOptions : public Options { bool abort_on_uncaught_exception = false; std::vector conditions; bool detect_module = false; + bool print_required_tla = false; + bool require_module = false; std::string dns_result_order; bool enable_source_maps = false; bool experimental_fetch = true; diff --git a/test/es-module/test-esm-cjs-load-error-note.mjs b/test/es-module/test-esm-cjs-load-error-note.mjs index 4df9e903eb627a..00ff5582c71bda 100644 --- a/test/es-module/test-esm-cjs-load-error-note.mjs +++ b/test/es-module/test-esm-cjs-load-error-note.mjs @@ -6,16 +6,16 @@ import { describe, it } from 'node:test'; // Expect note to be included in the error output -const expectedNote = 'To load an ES module, ' + -'set "type": "module" in the package.json ' + -'or use the .mjs extension.'; +// Don't match the following sentence because it can change as features are +// added. +const expectedNote = 'Warning: To load an ES module'; const mustIncludeMessage = { - getMessage: () => (stderr) => `${expectedNote} not found in ${stderr}`, + getMessage: (stderr) => `${expectedNote} not found in ${stderr}`, includeNote: true, }; const mustNotIncludeMessage = { - getMessage: () => (stderr) => `${expectedNote} must not be included in ${stderr}`, + getMessage: (stderr) => `${expectedNote} must not be included in ${stderr}`, includeNote: false, }; diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js index 860775df0a2ce8..83125fce738139 100644 --- a/test/es-module/test-esm-loader-modulemap.js +++ b/test/es-module/test-esm-loader-modulemap.js @@ -6,7 +6,7 @@ require('../common'); const { strictEqual, throws } = require('assert'); const { createModuleLoader } = require('internal/modules/esm/loader'); const { LoadCache, ResolveCache } = require('internal/modules/esm/module_map'); -const ModuleJob = require('internal/modules/esm/module_job'); +const { ModuleJob } = require('internal/modules/esm/module_job'); const createDynamicModule = require( 'internal/modules/esm/create_dynamic_module'); diff --git a/test/es-module/test-require-module-cached-tla.js b/test/es-module/test-require-module-cached-tla.js new file mode 100644 index 00000000000000..d98b012c349aa1 --- /dev/null +++ b/test/es-module/test-require-module-cached-tla.js @@ -0,0 +1,14 @@ +// Flags: --experimental-require-module +'use strict'; + +const common = require('../common'); +const assert = require('assert'); + +(async () => { + await import('../fixtures/es-modules/tla/resolved.mjs'); + assert.throws(() => { + require('../fixtures/es-modules/tla/resolved.mjs'); + }, { + code: 'ERR_REQUIRE_ASYNC_MODULE', + }); +})().then(common.mustCall()); diff --git a/test/es-module/test-require-module-conditional-exports.js b/test/es-module/test-require-module-conditional-exports.js new file mode 100644 index 00000000000000..354c8b72abc7a1 --- /dev/null +++ b/test/es-module/test-require-module-conditional-exports.js @@ -0,0 +1,35 @@ +// Flags: --experimental-require-module +'use strict'; + +require('../common'); +const assert = require('assert'); +const { isModuleNamespaceObject } = require('util/types'); + +// If only "require" exports are defined, return "require" exports. +{ + const mod = require('../fixtures/es-modules/exports-require-only/load.cjs'); + assert.deepStrictEqual({ ...mod }, { type: 'cjs' }); + assert(!isModuleNamespaceObject(mod)); +} + +// If only "import" exports are defined, throw ERR_PACKAGE_PATH_NOT_EXPORTED +// instead of falling back to it, because the request comes from require(). +assert.throws(() => { + require('../fixtures/es-modules/exports-import-only/load.cjs'); +}, { + code: 'ERR_PACKAGE_PATH_NOT_EXPORTED' +}); + +// If both are defined, "require" is used. +{ + const mod = require('../fixtures/es-modules/exports-both/load.cjs'); + assert.deepStrictEqual({ ...mod }, { type: 'cjs' }); + assert(!isModuleNamespaceObject(mod)); +} + +// If "import" and "default" are defined, "default" is used. +{ + const mod = require('../fixtures/es-modules/exports-import-default/load.cjs'); + assert.deepStrictEqual({ ...mod }, { type: 'cjs' }); + assert(!isModuleNamespaceObject(mod)); +} diff --git a/test/es-module/test-require-module-default-extension.js b/test/es-module/test-require-module-default-extension.js new file mode 100644 index 00000000000000..7c49e21aba9a15 --- /dev/null +++ b/test/es-module/test-require-module-default-extension.js @@ -0,0 +1,17 @@ +// Flags: --experimental-require-module +'use strict'; + +require('../common'); +const assert = require('assert'); +const { isModuleNamespaceObject } = require('util/types'); + +const mod = require('../fixtures/es-modules/package-default-extension/index.mjs'); +assert.deepStrictEqual({ ...mod }, { entry: 'mjs' }); +assert(isModuleNamespaceObject(mod)); + +assert.throws(() => { + const mod = require('../fixtures/es-modules/package-default-extension'); + console.log(mod); // In case it succeeds, log the result for debugging. +}, { + code: 'MODULE_NOT_FOUND', +}); diff --git a/test/es-module/test-require-module-dynamic-import-1.js b/test/es-module/test-require-module-dynamic-import-1.js new file mode 100644 index 00000000000000..000e31485f559e --- /dev/null +++ b/test/es-module/test-require-module-dynamic-import-1.js @@ -0,0 +1,32 @@ +// Flags: --experimental-require-module +'use strict'; + +// Tests that previously dynamically import()'ed results are reference equal to +// require()'d results. +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const { pathToFileURL } = require('url'); + +(async () => { + const modules = [ + '../fixtures/es-module-loaders/module-named-exports.mjs', + '../fixtures/es-modules/import-esm.mjs', + '../fixtures/es-modules/require-cjs.mjs', + '../fixtures/es-modules/cjs-exports.mjs', + '../common/index.mjs', + '../fixtures/es-modules/package-type-module/index.js', + ]; + for (const id of modules) { + const url = pathToFileURL(path.resolve(__dirname, id)); + const imported = await import(url); + const required = require(id); + assert.strictEqual(imported, required, + `import()'ed and require()'ed result of ${id} was not reference equal`); + } + + const id = '../fixtures/es-modules/data-import.mjs'; + const imported = await import(id); + const required = require(id); + assert.strictEqual(imported.data, required.data); +})().then(common.mustCall()); diff --git a/test/es-module/test-require-module-dynamic-import-2.js b/test/es-module/test-require-module-dynamic-import-2.js new file mode 100644 index 00000000000000..6c31c04f0b2e77 --- /dev/null +++ b/test/es-module/test-require-module-dynamic-import-2.js @@ -0,0 +1,32 @@ +// Flags: --experimental-require-module +'use strict'; + +// Tests that previously dynamically require()'ed results are reference equal to +// import()'d results. +const common = require('../common'); +const assert = require('assert'); +const { pathToFileURL } = require('url'); +const path = require('path'); + +(async () => { + const modules = [ + '../fixtures/es-module-loaders/module-named-exports.mjs', + '../fixtures/es-modules/import-esm.mjs', + '../fixtures/es-modules/require-cjs.mjs', + '../fixtures/es-modules/cjs-exports.mjs', + '../common/index.mjs', + '../fixtures/es-modules/package-type-module/index.js', + ]; + for (const id of modules) { + const url = pathToFileURL(path.resolve(__dirname, id)); + const required = require(id); + const imported = await import(url); + assert.strictEqual(imported, required, + `import()'ed and require()'ed result of ${id} was not reference equal`); + } + + const id = '../fixtures/es-modules/data-import.mjs'; + const required = require(id); + const imported = await import(id); + assert.strictEqual(imported.data, required.data); +})().then(common.mustCall()); diff --git a/test/es-module/test-require-module-errors.js b/test/es-module/test-require-module-errors.js new file mode 100644 index 00000000000000..b54f8f96af3006 --- /dev/null +++ b/test/es-module/test-require-module-errors.js @@ -0,0 +1,48 @@ +// Flags: --experimental-require-module +'use strict'; + +require('../common'); +const assert = require('assert'); +const { spawnSyncAndExit } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +spawnSyncAndExit(process.execPath, [ + '--experimental-require-module', + fixtures.path('es-modules/require-syntax-error.cjs'), +], { + status: 1, + signal: null, + stderr(output) { + assert.match(output, /var foo bar;/); + assert.match(output, /SyntaxError: Unexpected identifier 'bar'/); + return true; + }, +}); + +spawnSyncAndExit(process.execPath, [ + '--experimental-require-module', + fixtures.path('es-modules/require-reference-error.cjs'), +], { + status: 1, + signal: null, + trim: true, + stdout: 'executed', + stderr(output) { + assert.match(output, /module\.exports = { hello: 'world' };/); + assert.match(output, /ReferenceError: module is not defined/); + return true; + }, +}); + +spawnSyncAndExit(process.execPath, [ + '--experimental-require-module', + fixtures.path('es-modules/require-throw-error.cjs'), +], { + status: 1, + signal: null, + stderr(output) { + assert.match(output, /throw new Error\('test'\);/); + assert.match(output, /Error: test/); + return true; + }, +}); diff --git a/test/es-module/test-require-module-implicit.js b/test/es-module/test-require-module-implicit.js new file mode 100644 index 00000000000000..5b5a4a4bbb47b0 --- /dev/null +++ b/test/es-module/test-require-module-implicit.js @@ -0,0 +1,33 @@ +// Flags: --experimental-require-module +'use strict'; + +// Tests that require()ing modules without explicit module type information +// warns and errors. +require('../common'); +const assert = require('assert'); +const { isModuleNamespaceObject } = require('util/types'); + +assert.throws(() => { + require('../fixtures/es-modules/package-without-type/noext-esm'); +}, { + message: /Unexpected token 'export'/ +}); + +assert.throws(() => { + require('../fixtures/es-modules/loose.js'); +}, { + message: /Unexpected token 'export'/ +}); + +{ + // .mjs should not be matched as default extensions. + const id = '../fixtures/es-modules/should-not-be-resolved'; + assert.throws(() => { + require(id); + }, { + code: 'MODULE_NOT_FOUND' + }); + const mod = require(`${id}.mjs`); + assert.deepStrictEqual({ ...mod }, { hello: 'world' }); + assert(isModuleNamespaceObject(mod)); +} diff --git a/test/es-module/test-require-module-preload.js b/test/es-module/test-require-module-preload.js new file mode 100644 index 00000000000000..cd51e201b63df8 --- /dev/null +++ b/test/es-module/test-require-module-preload.js @@ -0,0 +1,72 @@ +'use strict'; + +require('../common'); +const { spawnSyncAndExitWithoutError } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const stderr = /ExperimentalWarning: Support for loading ES Module in require/; + +// Test named exports. +{ + spawnSyncAndExitWithoutError( + process.execPath, + [ '--experimental-require-module', '-r', fixtures.path('../fixtures/es-module-loaders/module-named-exports.mjs') ], + { + stderr, + } + ); +} + +// Test ESM that import ESM. +{ + spawnSyncAndExitWithoutError( + process.execPath, + [ '--experimental-require-module', '-r', fixtures.path('../fixtures/es-modules/import-esm.mjs') ], + { + stderr, + stdout: 'world', + trim: true, + } + ); +} + +// Test ESM that import CJS. +{ + spawnSyncAndExitWithoutError( + process.execPath, + [ '--experimental-require-module', '-r', fixtures.path('../fixtures/es-modules/cjs-exports.mjs') ], + { + stdout: 'ok', + stderr, + trim: true, + } + ); +} + +// Test ESM that require() CJS. +// Can't use the common/index.mjs here because that checks the globals, and +// -r injects a bunch of globals. +{ + spawnSyncAndExitWithoutError( + process.execPath, + [ '--experimental-require-module', '-r', fixtures.path('../fixtures/es-modules/require-cjs.mjs') ], + { + stdout: 'world', + stderr, + trim: true, + } + ); +} + +// Test "type": "module" and "main" field in package.json. +{ + spawnSyncAndExitWithoutError( + process.execPath, + [ '--experimental-require-module', '-r', fixtures.path('../fixtures/es-modules/package-type-module') ], + { + stdout: 'package-type-module', + stderr, + trim: true, + } + ); +} diff --git a/test/es-module/test-require-module-special-import.js b/test/es-module/test-require-module-special-import.js new file mode 100644 index 00000000000000..3ff03d08e8d1d0 --- /dev/null +++ b/test/es-module/test-require-module-special-import.js @@ -0,0 +1,11 @@ +// Flags: --experimental-require-module +'use strict'; + +require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/network-import.mjs'); +}, { + code: 'ERR_NETWORK_IMPORT_DISALLOWED' +}); diff --git a/test/es-module/test-require-module-tla.js b/test/es-module/test-require-module-tla.js new file mode 100644 index 00000000000000..9b38b1cab3fcb5 --- /dev/null +++ b/test/es-module/test-require-module-tla.js @@ -0,0 +1,63 @@ +// Flags: --experimental-require-module +'use strict'; + +require('../common'); +const assert = require('assert'); +const { spawnSyncAndExit } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const message = /require\(\) cannot be used on an ESM graph with top-level await/; +const code = 'ERR_REQUIRE_ASYNC_MODULE'; + +assert.throws(() => { + require('../fixtures/es-modules/tla/rejected.mjs'); +}, { message, code }); + +assert.throws(() => { + require('../fixtures/es-modules/tla/unresolved.mjs'); +}, { message, code }); + + +assert.throws(() => { + require('../fixtures/es-modules/tla/resolved.mjs'); +}, { message, code }); + +// Test TLA in inner graphs. +assert.throws(() => { + require('../fixtures/es-modules/tla/parent.mjs'); +}, { message, code }); + +{ + spawnSyncAndExit(process.execPath, [ + '--experimental-require-module', + fixtures.path('es-modules/tla/require-execution.js'), + ], { + signal: null, + status: 1, + stderr(output) { + assert.doesNotMatch(output, /I am executed/); + assert.match(output, message); + return true; + }, + stdout: '' + }); +} + +{ + spawnSyncAndExit(process.execPath, [ + '--experimental-require-module', + '--experimental-print-required-tla', + fixtures.path('es-modules/tla/require-execution.js'), + ], { + signal: null, + status: 1, + stderr(output) { + assert.match(output, /I am executed/); + assert.match(output, /Error: unexpected top-level await at.*execution\.mjs:3/); + assert.match(output, /await Promise\.resolve\('hi'\)/); + assert.match(output, message); + return true; + }, + stdout: '' + }); +} diff --git a/test/es-module/test-require-module-twice.js b/test/es-module/test-require-module-twice.js new file mode 100644 index 00000000000000..5312cda8506a6e --- /dev/null +++ b/test/es-module/test-require-module-twice.js @@ -0,0 +1,21 @@ +// Flags: --experimental-require-module +'use strict'; + +require('../common'); +const assert = require('assert'); + +const modules = [ + '../fixtures/es-module-loaders/module-named-exports.mjs', + '../fixtures/es-modules/import-esm.mjs', + '../fixtures/es-modules/require-cjs.mjs', + '../fixtures/es-modules/cjs-exports.mjs', + '../common/index.mjs', + '../fixtures/es-modules/package-type-module/index.js', +]; + +for (const id of modules) { + const first = require(id); + const second = require(id); + assert.strictEqual(first, second, + `the results of require('${id}') twice are not reference equal`); +} diff --git a/test/es-module/test-require-module.js b/test/es-module/test-require-module.js new file mode 100644 index 00000000000000..631f5d731a5c86 --- /dev/null +++ b/test/es-module/test-require-module.js @@ -0,0 +1,62 @@ +// Flags: --experimental-require-module +'use strict'; + +const common = require('../common'); +const assert = require('assert'); +const { isModuleNamespaceObject } = require('util/types'); + +common.expectWarning( + 'ExperimentalWarning', + 'Support for loading ES Module in require() is an experimental feature ' + + 'and might change at any time' +); + +// Test named exports. +{ + const mod = require('../fixtures/es-module-loaders/module-named-exports.mjs'); + assert.deepStrictEqual({ ...mod }, { foo: 'foo', bar: 'bar' }); + assert(isModuleNamespaceObject(mod)); +} + +// Test ESM that import ESM. +{ + const mod = require('../fixtures/es-modules/import-esm.mjs'); + assert.deepStrictEqual({ ...mod }, { hello: 'world' }); + assert(isModuleNamespaceObject(mod)); +} + +// Test ESM that import CJS. +{ + const mod = require('../fixtures/es-modules/cjs-exports.mjs'); + assert.deepStrictEqual({ ...mod }, {}); + assert(isModuleNamespaceObject(mod)); +} + +// Test ESM that require() CJS. +{ + const mjs = require('../common/index.mjs'); + // Only comparing a few properties because the ESM version of test/common doesn't + // re-export everything from the CJS version. + assert.strictEqual(common.mustCall, mjs.mustCall); + assert.strictEqual(common.localIPv6Hosts, mjs.localIPv6Hosts); + assert(!isModuleNamespaceObject(common)); + assert(isModuleNamespaceObject(mjs)); +} + +// Test "type": "module" and "main" field in package.json. +// Also, test default export. +{ + const mod = require('../fixtures/es-modules/package-type-module'); + assert.deepStrictEqual({ ...mod }, { default: 'package-type-module' }); + assert(isModuleNamespaceObject(mod)); +} + +// Test data: import. +{ + const mod = require('../fixtures/es-modules/data-import.mjs'); + assert.deepStrictEqual({ ...mod }, { + data: { hello: 'world' }, + id: 'data:text/javascript,export default %7B%20hello%3A%20%22world%22%20%7D' + }); + assert(isModuleNamespaceObject(mod)); +} diff --git a/test/fixtures/es-modules/data-import.mjs b/test/fixtures/es-modules/data-import.mjs new file mode 100644 index 00000000000000..e67c0b4696e139 --- /dev/null +++ b/test/fixtures/es-modules/data-import.mjs @@ -0,0 +1,2 @@ +export { default as data } from 'data:text/javascript,export default %7B%20hello%3A%20%22world%22%20%7D'; +export const id = 'data:text/javascript,export default %7B%20hello%3A%20%22world%22%20%7D'; diff --git a/test/fixtures/es-modules/deprecated-folders-ignore/package.json b/test/fixtures/es-modules/deprecated-folders-ignore/package.json index 52a3a1e8a8b787..3dbc1ca591c055 100644 --- a/test/fixtures/es-modules/deprecated-folders-ignore/package.json +++ b/test/fixtures/es-modules/deprecated-folders-ignore/package.json @@ -1,4 +1,3 @@ { "type": "module" } - diff --git a/test/fixtures/es-modules/exports-both/load.cjs b/test/fixtures/es-modules/exports-both/load.cjs new file mode 100644 index 00000000000000..8b01d84f780b49 --- /dev/null +++ b/test/fixtures/es-modules/exports-both/load.cjs @@ -0,0 +1 @@ +module.exports = require('dep'); diff --git a/test/fixtures/es-modules/exports-both/node_modules/dep/mod.cjs b/test/fixtures/es-modules/exports-both/node_modules/dep/mod.cjs new file mode 100644 index 00000000000000..267c18747d99bf --- /dev/null +++ b/test/fixtures/es-modules/exports-both/node_modules/dep/mod.cjs @@ -0,0 +1 @@ +module.exports = { type: "cjs" }; diff --git a/test/fixtures/es-modules/exports-both/node_modules/dep/mod.mjs b/test/fixtures/es-modules/exports-both/node_modules/dep/mod.mjs new file mode 100644 index 00000000000000..ba57355b40d5d0 --- /dev/null +++ b/test/fixtures/es-modules/exports-both/node_modules/dep/mod.mjs @@ -0,0 +1,2 @@ +export const type = "mjs"; + diff --git a/test/fixtures/es-modules/exports-both/node_modules/dep/package.json b/test/fixtures/es-modules/exports-both/node_modules/dep/package.json new file mode 100644 index 00000000000000..d4be8cb35eda9f --- /dev/null +++ b/test/fixtures/es-modules/exports-both/node_modules/dep/package.json @@ -0,0 +1,8 @@ +{ + "exports": { + ".": { + "import": "./mod.mjs", + "require": "./mod.cjs" + } + } +} diff --git a/test/fixtures/es-modules/exports-import-default/load.cjs b/test/fixtures/es-modules/exports-import-default/load.cjs new file mode 100644 index 00000000000000..8b01d84f780b49 --- /dev/null +++ b/test/fixtures/es-modules/exports-import-default/load.cjs @@ -0,0 +1 @@ +module.exports = require('dep'); diff --git a/test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.js b/test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.js new file mode 100644 index 00000000000000..267c18747d99bf --- /dev/null +++ b/test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.js @@ -0,0 +1 @@ +module.exports = { type: "cjs" }; diff --git a/test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.mjs b/test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.mjs new file mode 100644 index 00000000000000..ba57355b40d5d0 --- /dev/null +++ b/test/fixtures/es-modules/exports-import-default/node_modules/dep/mod.mjs @@ -0,0 +1,2 @@ +export const type = "mjs"; + diff --git a/test/fixtures/es-modules/exports-import-default/node_modules/dep/package.json b/test/fixtures/es-modules/exports-import-default/node_modules/dep/package.json new file mode 100644 index 00000000000000..de60e166d628a8 --- /dev/null +++ b/test/fixtures/es-modules/exports-import-default/node_modules/dep/package.json @@ -0,0 +1,8 @@ +{ + "exports": { + ".": { + "import": "./mod.mjs", + "default": "./mod.js" + } + } +} diff --git a/test/fixtures/es-modules/exports-import-only/load.cjs b/test/fixtures/es-modules/exports-import-only/load.cjs new file mode 100644 index 00000000000000..ec9c535a04352d --- /dev/null +++ b/test/fixtures/es-modules/exports-import-only/load.cjs @@ -0,0 +1,2 @@ +module.exports = require('dep'); + diff --git a/test/fixtures/es-modules/exports-import-only/node_modules/dep/mod.js b/test/fixtures/es-modules/exports-import-only/node_modules/dep/mod.js new file mode 100644 index 00000000000000..e25304b3a9d21e --- /dev/null +++ b/test/fixtures/es-modules/exports-import-only/node_modules/dep/mod.js @@ -0,0 +1,2 @@ +export const type = 'mjs'; + diff --git a/test/fixtures/es-modules/exports-import-only/node_modules/dep/package.json b/test/fixtures/es-modules/exports-import-only/node_modules/dep/package.json new file mode 100644 index 00000000000000..7b208f585f5a07 --- /dev/null +++ b/test/fixtures/es-modules/exports-import-only/node_modules/dep/package.json @@ -0,0 +1,8 @@ +{ + "type": "module", + "exports": { + ".": { + "import": "./mod.js" + } + } +} diff --git a/test/fixtures/es-modules/exports-require-only/load.cjs b/test/fixtures/es-modules/exports-require-only/load.cjs new file mode 100644 index 00000000000000..8b01d84f780b49 --- /dev/null +++ b/test/fixtures/es-modules/exports-require-only/load.cjs @@ -0,0 +1 @@ +module.exports = require('dep'); diff --git a/test/fixtures/es-modules/exports-require-only/node_modules/dep/mod.js b/test/fixtures/es-modules/exports-require-only/node_modules/dep/mod.js new file mode 100644 index 00000000000000..267c18747d99bf --- /dev/null +++ b/test/fixtures/es-modules/exports-require-only/node_modules/dep/mod.js @@ -0,0 +1 @@ +module.exports = { type: "cjs" }; diff --git a/test/fixtures/es-modules/exports-require-only/node_modules/dep/package.json b/test/fixtures/es-modules/exports-require-only/node_modules/dep/package.json new file mode 100644 index 00000000000000..134ee945df7d20 --- /dev/null +++ b/test/fixtures/es-modules/exports-require-only/node_modules/dep/package.json @@ -0,0 +1,7 @@ +{ + "exports": { + ".": { + "require": "./mod.js" + } + } +} diff --git a/test/fixtures/es-modules/import-esm.mjs b/test/fixtures/es-modules/import-esm.mjs new file mode 100644 index 00000000000000..d8c0d983dda3d2 --- /dev/null +++ b/test/fixtures/es-modules/import-esm.mjs @@ -0,0 +1,3 @@ +import { hello } from './imported-esm.mjs'; +console.log(hello); +export { hello }; diff --git a/test/fixtures/es-modules/imported-esm.mjs b/test/fixtures/es-modules/imported-esm.mjs new file mode 100644 index 00000000000000..35f468bf4856d8 --- /dev/null +++ b/test/fixtures/es-modules/imported-esm.mjs @@ -0,0 +1 @@ +export const hello = 'world'; diff --git a/test/fixtures/es-modules/network-import.mjs b/test/fixtures/es-modules/network-import.mjs new file mode 100644 index 00000000000000..529d563b4d982f --- /dev/null +++ b/test/fixtures/es-modules/network-import.mjs @@ -0,0 +1 @@ +import 'http://example.com/foo.js'; diff --git a/test/fixtures/es-modules/package-default-extension/index.cjs b/test/fixtures/es-modules/package-default-extension/index.cjs new file mode 100644 index 00000000000000..cb312f0f7dac8b --- /dev/null +++ b/test/fixtures/es-modules/package-default-extension/index.cjs @@ -0,0 +1 @@ +module.exports = { entry: 'cjs' }; diff --git a/test/fixtures/es-modules/package-default-extension/index.mjs b/test/fixtures/es-modules/package-default-extension/index.mjs new file mode 100644 index 00000000000000..aac4141efdb58a --- /dev/null +++ b/test/fixtures/es-modules/package-default-extension/index.mjs @@ -0,0 +1 @@ +export const entry = 'mjs'; diff --git a/test/fixtures/es-modules/reference-error.mjs b/test/fixtures/es-modules/reference-error.mjs new file mode 100644 index 00000000000000..15be06aad9a6d5 --- /dev/null +++ b/test/fixtures/es-modules/reference-error.mjs @@ -0,0 +1,3 @@ +// Reference errors are not thrown until reference happens. +console.log('executed'); +module.exports = { hello: 'world' }; diff --git a/test/fixtures/es-modules/require-cjs.mjs b/test/fixtures/es-modules/require-cjs.mjs new file mode 100644 index 00000000000000..a79a9b004c2237 --- /dev/null +++ b/test/fixtures/es-modules/require-cjs.mjs @@ -0,0 +1,5 @@ +import { createRequire } from "node:module"; +const require = createRequire(import.meta.url); +const exports = require('./required-cjs'); +console.log(exports.hello); +export default exports; diff --git a/test/fixtures/es-modules/require-reference-error.cjs b/test/fixtures/es-modules/require-reference-error.cjs new file mode 100644 index 00000000000000..9d90c022cc1cc4 --- /dev/null +++ b/test/fixtures/es-modules/require-reference-error.cjs @@ -0,0 +1,2 @@ +'use strict'; +require('./reference-error.mjs'); diff --git a/test/fixtures/es-modules/require-syntax-error.cjs b/test/fixtures/es-modules/require-syntax-error.cjs new file mode 100644 index 00000000000000..918dc06f369c73 --- /dev/null +++ b/test/fixtures/es-modules/require-syntax-error.cjs @@ -0,0 +1,2 @@ +'use strict'; +require('./syntax-error.mjs'); diff --git a/test/fixtures/es-modules/require-throw-error.cjs b/test/fixtures/es-modules/require-throw-error.cjs new file mode 100644 index 00000000000000..6fdedc5400ce5a --- /dev/null +++ b/test/fixtures/es-modules/require-throw-error.cjs @@ -0,0 +1,2 @@ +'use strict'; +require('./throw-error.mjs'); diff --git a/test/fixtures/es-modules/required-cjs.js b/test/fixtures/es-modules/required-cjs.js new file mode 100644 index 00000000000000..69f1fd8c3776c1 --- /dev/null +++ b/test/fixtures/es-modules/required-cjs.js @@ -0,0 +1,3 @@ +module.exports = { + hello: 'world', +}; diff --git a/test/fixtures/es-modules/should-not-be-resolved.mjs b/test/fixtures/es-modules/should-not-be-resolved.mjs new file mode 100644 index 00000000000000..35f468bf4856d8 --- /dev/null +++ b/test/fixtures/es-modules/should-not-be-resolved.mjs @@ -0,0 +1 @@ +export const hello = 'world'; diff --git a/test/fixtures/es-modules/syntax-error.mjs b/test/fixtures/es-modules/syntax-error.mjs new file mode 100644 index 00000000000000..c2cd118b23b133 --- /dev/null +++ b/test/fixtures/es-modules/syntax-error.mjs @@ -0,0 +1 @@ +var foo bar; diff --git a/test/fixtures/es-modules/throw-error.mjs b/test/fixtures/es-modules/throw-error.mjs new file mode 100644 index 00000000000000..bc9eaa01354ace --- /dev/null +++ b/test/fixtures/es-modules/throw-error.mjs @@ -0,0 +1 @@ +throw new Error('test'); diff --git a/test/fixtures/es-modules/tla/execution.mjs b/test/fixtures/es-modules/tla/execution.mjs new file mode 100644 index 00000000000000..c060945ba02f21 --- /dev/null +++ b/test/fixtures/es-modules/tla/execution.mjs @@ -0,0 +1,3 @@ +import process from 'node:process'; +process._rawDebug('I am executed'); +await Promise.resolve('hi'); diff --git a/test/fixtures/es-modules/tla/require-execution.js b/test/fixtures/es-modules/tla/require-execution.js new file mode 100644 index 00000000000000..8d3ec9d107a838 --- /dev/null +++ b/test/fixtures/es-modules/tla/require-execution.js @@ -0,0 +1 @@ +require('./execution.mjs'); diff --git a/test/fixtures/es-modules/tla/resolved.mjs b/test/fixtures/es-modules/tla/resolved.mjs new file mode 100644 index 00000000000000..ff717caf5a2e3c --- /dev/null +++ b/test/fixtures/es-modules/tla/resolved.mjs @@ -0,0 +1 @@ +await Promise.resolve('hello'); From 2e86c96833a4d36edf04af70ce68f9d02be173b4 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 4 Apr 2024 00:31:48 +0100 Subject: [PATCH 2/6] module: centralize SourceTextModule compilation for builtin loader This refactors the code that compiles SourceTextModule for the built-in ESM loader to use a common routine so that it's easier to customize cache handling for the ESM loader. In addition this introduces a common symbol for import.meta and import() so that we don't need to create additional closures as handlers, since we can get all the information we need from the V8 callback already. This should reduce the memory footprint of ESM as well. PR-URL: https://github.com/nodejs/node/pull/52291 Refs: https://github.com/nodejs/node/issues/47472 Reviewed-By: Geoffrey Booth Reviewed-By: Stephen Belanger --- .../modules/esm/create_dynamic_module.js | 5 +- lib/internal/modules/esm/loader.js | 34 +------- lib/internal/modules/esm/translators.js | 26 +----- lib/internal/modules/esm/utils.js | 81 ++++++++++++++++--- lib/internal/vm/module.js | 11 ++- src/env_properties.h | 1 + src/module_wrap.cc | 42 ++++++---- 7 files changed, 109 insertions(+), 91 deletions(-) diff --git a/lib/internal/modules/esm/create_dynamic_module.js b/lib/internal/modules/esm/create_dynamic_module.js index d4f5a85db95f77..26e21d8407c729 100644 --- a/lib/internal/modules/esm/create_dynamic_module.js +++ b/lib/internal/modules/esm/create_dynamic_module.js @@ -55,8 +55,8 @@ ${ArrayPrototypeJoin(ArrayPrototypeMap(imports, createImport), '\n')} ${ArrayPrototypeJoin(ArrayPrototypeMap(exports, createExport), '\n')} import.meta.done(); `; - const { ModuleWrap } = internalBinding('module_wrap'); - const m = new ModuleWrap(`${url}`, undefined, source, 0, 0); + const { registerModule, compileSourceTextModule } = require('internal/modules/esm/utils'); + const m = compileSourceTextModule(`${url}`, source); const readyfns = new SafeSet(); /** @type {DynamicModuleReflect} */ @@ -68,7 +68,6 @@ import.meta.done(); if (imports.length) { reflect.imports = { __proto__: null }; } - const { registerModule } = require('internal/modules/esm/utils'); registerModule(m, { __proto__: null, initializeImportMeta: (meta, wrap) => { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index bdcd6028dacb39..d39bd37d801d42 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -25,13 +25,10 @@ const { getOptionValue } = require('internal/options'); const { isURL, pathToFileURL, URL } = require('internal/url'); const { emitExperimentalWarning, kEmptyObject } = require('internal/util'); const { - registerModule, + compileSourceTextModule, getDefaultConditions, } = require('internal/modules/esm/utils'); const { kImplicitAssertType } = require('internal/modules/esm/assert'); -const { - maybeCacheSourceMap, -} = require('internal/source_map/source_map_cache'); const { canParse } = internalBinding('url'); const { ModuleWrap } = internalBinding('module_wrap'); let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; @@ -193,16 +190,7 @@ class ModuleLoader { async eval(source, url) { const evalInstance = (url) => { - const module = new ModuleWrap(url, undefined, source, 0, 0); - registerModule(module, { - __proto__: null, - initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), - importModuleDynamically: (specifier, { url }, importAttributes) => { - return this.import(specifier, url, importAttributes); - }, - }); - - return module; + return compileSourceTextModule(url, source, this); }; const { ModuleJob } = require('internal/modules/esm/module_job'); const job = new ModuleJob( @@ -273,26 +261,10 @@ class ModuleLoader { if (job !== undefined) { return job.module.getNamespaceSync(); } - // TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the // cache here, or use a carrier object to carry the compiled module script // into the constructor to ensure cache hit. - const wrap = new ModuleWrap(url, undefined, source, 0, 0); - // Cache the source map for the module if present. - if (wrap.sourceMapURL) { - maybeCacheSourceMap(url, source, null, false, undefined, wrap.sourceMapURL); - } - const { registerModule } = require('internal/modules/esm/utils'); - // TODO(joyeecheung): refactor so that the default options are shared across - // the built-in loaders. - registerModule(wrap, { - __proto__: null, - initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), - importModuleDynamically: (specifier, wrap, importAttributes) => { - return this.import(specifier, url, importAttributes); - }, - }); - + const wrap = compileSourceTextModule(url, source, this); const inspectBrk = (isMain && getOptionValue('--inspect-brk')); const { ModuleJobSync } = require('internal/modules/esm/module_job'); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 7312bd0b09f41a..4614b37570c65b 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -156,35 +156,13 @@ function errPath(url) { return url; } -/** - * Dynamically imports a module using the ESM loader. - * @param {string} specifier - The module specifier to import. - * @param {object} options - An object containing options for the import. - * @param {string} options.url - The URL of the module requesting the import. - * @param {Record} [attributes] - An object containing attributes for the import. - * @returns {Promise} The imported module. - */ -async function importModuleDynamically(specifier, { url }, attributes) { - const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); - return cascadedLoader.import(specifier, url, attributes); -} - // Strategy for loading a standard JavaScript module. translators.set('module', function moduleStrategy(url, source, isMain) { assertBufferSource(source, true, 'load'); source = stringify(source); debug(`Translating StandardModule ${url}`); - const module = new ModuleWrap(url, undefined, source, 0, 0); - // Cache the source map for the module if present. - if (module.sourceMapURL) { - maybeCacheSourceMap(url, source, null, false, undefined, module.sourceMapURL); - } - const { registerModule } = require('internal/modules/esm/utils'); - registerModule(module, { - __proto__: null, - initializeImportMeta: (meta, wrap) => this.importMetaInitialize(meta, { url }), - importModuleDynamically, - }); + const { compileSourceTextModule } = require('internal/modules/esm/utils'); + const module = compileSourceTextModule(url, source, this); return module; }); diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index d7867864bba714..150816057129c1 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -13,12 +13,18 @@ const { }, } = internalBinding('util'); const { + source_text_module_default_hdo, vm_dynamic_import_default_internal, vm_dynamic_import_main_context_default, vm_dynamic_import_missing_flag, vm_dynamic_import_no_callback, } = internalBinding('symbols'); +const { ModuleWrap } = internalBinding('module_wrap'); +const { + maybeCacheSourceMap, +} = require('internal/source_map/source_map_cache'); + const { ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING_FLAG, ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING, @@ -167,28 +173,55 @@ function registerModule(referrer, registry) { moduleRegistries.set(idSymbol, registry); } +/** + * Proxy the import meta handling to the default loader for source text modules. + * @param {Record} meta - The import.meta object to initialize. + * @param {ModuleWrap} wrap - The ModuleWrap of the SourceTextModule where `import.meta` is referenced. + */ +function defaultInitializeImportMetaForModule(meta, wrap) { + const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); + return cascadedLoader.importMetaInitialize(meta, { url: wrap.url }); +} + /** * Defines the `import.meta` object for a given module. * @param {symbol} symbol - Reference to the module. * @param {Record} meta - The import.meta object to initialize. + * @param {ModuleWrap} wrap - The ModuleWrap of the SourceTextModule where `import.meta` is referenced. */ -function initializeImportMetaObject(symbol, meta) { - if (moduleRegistries.has(symbol)) { - const { initializeImportMeta, callbackReferrer } = moduleRegistries.get(symbol); - if (initializeImportMeta !== undefined) { - meta = initializeImportMeta(meta, callbackReferrer); - } +function initializeImportMetaObject(symbol, meta, wrap) { + if (symbol === source_text_module_default_hdo) { + defaultInitializeImportMetaForModule(meta, wrap); + return; + } + const data = moduleRegistries.get(symbol); + assert(data, `import.meta registry not found for ${wrap.url}`); + const { initializeImportMeta, callbackReferrer } = data; + if (initializeImportMeta !== undefined) { + meta = initializeImportMeta(meta, callbackReferrer); } } /** - * Proxy the dynamic import to the default loader. + * Proxy the dynamic import handling to the default loader for source text modules. + * @param {string} specifier - The module specifier string. + * @param {Record} attributes - The import attributes object. + * @param {string|null|undefined} referrerName - name of the referrer. + * @returns {Promise} - The imported module object. + */ +function defaultImportModuleDynamicallyForModule(specifier, attributes, referrerName) { + const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); + return cascadedLoader.import(specifier, referrerName, attributes); +} + +/** + * Proxy the dynamic import to the default loader for classic scripts. * @param {string} specifier - The module specifier string. * @param {Record} attributes - The import attributes object. * @param {string|null|undefined} referrerName - name of the referrer. * @returns {Promise} - The imported module object. */ -function defaultImportModuleDynamically(specifier, attributes, referrerName) { +function defaultImportModuleDynamicallyForScript(specifier, attributes, referrerName) { const parentURL = normalizeReferrerURL(referrerName); const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); return cascadedLoader.import(specifier, parentURL, attributes); @@ -208,12 +241,16 @@ async function importModuleDynamicallyCallback(referrerSymbol, specifier, attrib // and fall back to the default loader. if (referrerSymbol === vm_dynamic_import_main_context_default) { emitExperimentalWarning('vm.USE_MAIN_CONTEXT_DEFAULT_LOADER'); - return defaultImportModuleDynamically(specifier, attributes, referrerName); + return defaultImportModuleDynamicallyForScript(specifier, attributes, referrerName); } // For script compiled internally that should use the default loader to handle dynamic // import, proxy the request to the default loader without the warning. if (referrerSymbol === vm_dynamic_import_default_internal) { - return defaultImportModuleDynamically(specifier, attributes, referrerName); + return defaultImportModuleDynamicallyForScript(specifier, attributes, referrerName); + } + // For SourceTextModules compiled internally, proxy the request to the default loader. + if (referrerSymbol === source_text_module_default_hdo) { + return defaultImportModuleDynamicallyForModule(specifier, attributes, referrerName); } if (moduleRegistries.has(referrerSymbol)) { @@ -288,6 +325,29 @@ async function initializeHooks() { return { __proto__: null, hooks, preloadScripts }; } +/** + * Compile a SourceTextModule for the built-in ESM loader. Register it for default + * source map and import.meta and dynamic import() handling if cascadedLoader is provided. + * @param {string} url URL of the module. + * @param {string} source Source code of the module. + * @param {typeof import('./loader.js').ModuleLoader|undefined} cascadedLoader If provided, + * register the module for default handling. + * @returns {ModuleWrap} + */ +function compileSourceTextModule(url, source, cascadedLoader) { + const hostDefinedOption = cascadedLoader ? source_text_module_default_hdo : undefined; + const wrap = new ModuleWrap(url, undefined, source, 0, 0, hostDefinedOption); + + if (!cascadedLoader) { + return wrap; + } + // Cache the source map for the module if present. + if (wrap.sourceMapURL) { + maybeCacheSourceMap(url, source, null, false, undefined, wrap.sourceMapURL); + } + return wrap; +} + module.exports = { registerModule, initializeESM, @@ -296,4 +356,5 @@ module.exports = { getConditionsSet, loaderWorkerId: 'internal/modules/esm/worker', forceDefaultLoader, + compileSourceTextModule, }; diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index 5db3d9535f3974..74c1870337fc30 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -131,6 +131,11 @@ class Module { importModuleDynamicallyWrap(options.importModuleDynamically) : undefined, }; + // This will take precedence over the referrer as the object being + // passed into the callbacks. + registry.callbackReferrer = this; + const { registerModule } = require('internal/modules/esm/utils'); + registerModule(this[kWrap], registry); } else { assert(syntheticEvaluationSteps); this[kWrap] = new ModuleWrap(identifier, context, @@ -138,12 +143,6 @@ class Module { syntheticEvaluationSteps); } - // This will take precedence over the referrer as the object being - // passed into the callbacks. - registry.callbackReferrer = this; - const { registerModule } = require('internal/modules/esm/utils'); - registerModule(this[kWrap], registry); - this[kContext] = context; } diff --git a/src/env_properties.h b/src/env_properties.h index c7eae579c4ec7d..70f49314eebfa5 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -45,6 +45,7 @@ V(onpskexchange_symbol, "onpskexchange") \ V(resource_symbol, "resource_symbol") \ V(trigger_async_id_symbol, "trigger_async_id_symbol") \ + V(source_text_module_default_hdo, "source_text_module_default_hdo") \ V(vm_dynamic_import_default_internal, "vm_dynamic_import_default_internal") \ V(vm_dynamic_import_main_context_default, \ "vm_dynamic_import_main_context_default") \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index c2e2aa37ddefc5..2379cdbd27b45b 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -102,8 +102,10 @@ ModuleWrap* ModuleWrap::GetFromModule(Environment* env, return nullptr; } -// new ModuleWrap(url, context, source, lineOffset, columnOffset) -// new ModuleWrap(url, context, exportNames, syntheticExecutionFunction) +// new ModuleWrap(url, context, source, lineOffset, columnOffset, cachedData) +// new ModuleWrap(url, context, source, lineOffset, columOffset, +// hostDefinedOption) new ModuleWrap(url, context, exportNames, +// syntheticExecutionFunction) void ModuleWrap::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); CHECK_GE(args.Length(), 3); @@ -132,22 +134,36 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { int column_offset = 0; bool synthetic = args[2]->IsArray(); + + Local host_defined_options = + PrimitiveArray::New(isolate, HostDefinedOptions::kLength); + Local id_symbol; if (synthetic) { // new ModuleWrap(url, context, exportNames, syntheticExecutionFunction) CHECK(args[3]->IsFunction()); } else { // new ModuleWrap(url, context, source, lineOffset, columOffset, cachedData) + // new ModuleWrap(url, context, source, lineOffset, columOffset, + // hostDefinedOption) CHECK(args[2]->IsString()); CHECK(args[3]->IsNumber()); line_offset = args[3].As()->Value(); CHECK(args[4]->IsNumber()); column_offset = args[4].As()->Value(); - } + if (args[5]->IsSymbol()) { + id_symbol = args[5].As(); + } else { + id_symbol = Symbol::New(isolate, url); + } + host_defined_options->Set(isolate, HostDefinedOptions::kID, id_symbol); - Local host_defined_options = - PrimitiveArray::New(isolate, HostDefinedOptions::kLength); - Local id_symbol = Symbol::New(isolate, url); - host_defined_options->Set(isolate, HostDefinedOptions::kID, id_symbol); + if (that->SetPrivate(context, + realm->isolate_data()->host_defined_option_symbol(), + id_symbol) + .IsNothing()) { + return; + } + } ShouldNotAbortOnUncaughtScope no_abort_scope(realm->env()); TryCatchScope try_catch(realm->env()); @@ -173,8 +189,7 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { SyntheticModuleEvaluationStepsCallback); } else { ScriptCompiler::CachedData* cached_data = nullptr; - if (!args[5]->IsUndefined()) { - CHECK(args[5]->IsArrayBufferView()); + if (args[5]->IsArrayBufferView()) { Local cached_data_buf = args[5].As(); uint8_t* data = static_cast(cached_data_buf->Buffer()->Data()); @@ -237,13 +252,6 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { return; } - if (that->SetPrivate(context, - realm->isolate_data()->host_defined_option_symbol(), - id_symbol) - .IsNothing()) { - return; - } - // Use the extras object as an object whose GetCreationContext() will be the // original `context`, since the `Context` itself strictly speaking cannot // be stored in an internal field. @@ -837,7 +845,7 @@ void ModuleWrap::HostInitializeImportMetaObjectCallback( return; } DCHECK(id->IsSymbol()); - Local args[] = {id, meta}; + Local args[] = {id, meta, wrap}; TryCatchScope try_catch(env); USE(callback->Call( context, Undefined(realm->isolate()), arraysize(args), args)); From 269b145f7240e3797192bccf662035359b425e67 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 8 Apr 2024 16:45:55 +0200 Subject: [PATCH 3/6] module: disallow CJS <-> ESM edges in a cycle from require(esm) This patch disallows CJS <-> ESM edges when they come from require(esm) requested in ESM evalaution. Drive-by: don't reuse the cache for imported CJS modules to stash source code of required ESM because the former is also used for cycle detection. PR-URL: https://github.com/nodejs/node/pull/52264 Fixes: https://github.com/nodejs/node/issues/52145 Reviewed-By: Geoffrey Booth Reviewed-By: Guy Bedford Reviewed-By: Antoine du Hamel --- doc/api/errors.md | 15 ++++ lib/internal/errors.js | 1 + lib/internal/modules/cjs/loader.js | 77 ++++++++++++----- lib/internal/modules/esm/load.js | 10 +++ lib/internal/modules/esm/loader.js | 77 ++++++++++++++--- lib/internal/modules/esm/translators.js | 15 ++-- lib/internal/modules/helpers.js | 10 +++ src/env_properties.h | 1 + src/module_wrap.cc | 19 +++-- ...st-require-module-cycle-esm-cjs-esm-esm.js | 56 +++++++++++++ .../test-require-module-cycle-esm-cjs-esm.js | 72 ++++++++++++++++ ...equire-module-cycle-esm-esm-cjs-esm-esm.js | 70 ++++++++++++++++ ...st-require-module-cycle-esm-esm-cjs-esm.js | 82 +++++++++++++++++++ .../es-modules/esm-cjs-esm-cycle/a.mjs | 3 + .../es-modules/esm-cjs-esm-cycle/b.cjs | 3 + .../esm-cjs-esm-cycle/require-a.cjs | 1 + .../esm-cjs-esm-cycle/require-b.cjs | 1 + .../es-modules/esm-cjs-esm-esm-cycle/a.mjs | 1 + .../es-modules/esm-cjs-esm-esm-cycle/b.cjs | 1 + .../es-modules/esm-cjs-esm-esm-cycle/c.mjs | 1 + .../es-modules/esm-esm-cjs-esm-cycle/a.mjs | 15 ++++ .../es-modules/esm-esm-cjs-esm-cycle/b.mjs | 3 + .../es-modules/esm-esm-cjs-esm-cycle/c.mjs | 5 ++ .../es-modules/esm-esm-cjs-esm-cycle/d.mjs | 3 + .../esm-esm-cjs-esm-esm-cycle/a.mjs | 1 + .../esm-esm-cjs-esm-esm-cycle/b.mjs | 1 + .../esm-esm-cjs-esm-esm-cycle/c.cjs | 1 + .../esm-esm-cjs-esm-esm-cycle/z.mjs | 1 + 28 files changed, 499 insertions(+), 47 deletions(-) create mode 100644 test/es-module/test-require-module-cycle-esm-cjs-esm-esm.js create mode 100644 test/es-module/test-require-module-cycle-esm-cjs-esm.js create mode 100644 test/es-module/test-require-module-cycle-esm-esm-cjs-esm-esm.js create mode 100644 test/es-module/test-require-module-cycle-esm-esm-cjs-esm.js create mode 100644 test/fixtures/es-modules/esm-cjs-esm-cycle/a.mjs create mode 100644 test/fixtures/es-modules/esm-cjs-esm-cycle/b.cjs create mode 100644 test/fixtures/es-modules/esm-cjs-esm-cycle/require-a.cjs create mode 100644 test/fixtures/es-modules/esm-cjs-esm-cycle/require-b.cjs create mode 100644 test/fixtures/es-modules/esm-cjs-esm-esm-cycle/a.mjs create mode 100644 test/fixtures/es-modules/esm-cjs-esm-esm-cycle/b.cjs create mode 100644 test/fixtures/es-modules/esm-cjs-esm-esm-cycle/c.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-cycle/a.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-cycle/b.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-cycle/c.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-cycle/d.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/a.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/b.mjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/c.cjs create mode 100644 test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/z.mjs diff --git a/doc/api/errors.md b/doc/api/errors.md index a2f59c74f44def..7a1d6d12b981c9 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2521,6 +2521,21 @@ Accessing `Object.prototype.__proto__` has been forbidden using [`Object.setPrototypeOf`][] should be used to get and set the prototype of an object. + + +### `ERR_REQUIRE_CYCLE_MODULE` + +> Stability: 1 - Experimental + +When trying to `require()` a [ES Module][] under `--experimental-require-module`, +a CommonJS to ESM or ESM to CommonJS edge participates in an immediate cycle. +This is not allowed because ES Modules cannot be evaluated while they are +already being evaluated. + +To avoid the cycle, the `require()` call involved in a cycle should not happen +at the top-level of either a ES Module (via `createRequire()`) or a CommonJS +module, and should be done lazily in an inner function. + ### `ERR_REQUIRE_ASYNC_MODULE` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 101bd3a003a2b4..64e95ffa603208 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1692,6 +1692,7 @@ E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => { E('ERR_PERFORMANCE_INVALID_TIMESTAMP', '%d is not a valid timestamp', TypeError); E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError); +E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error); E('ERR_REQUIRE_ESM', function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { hideInternalStackFrames(this); diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 34bec05ff72455..1b5dc834953534 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -63,24 +63,33 @@ const { Symbol, } = primordials; +const { kEvaluated } = internalBinding('module_wrap'); + // Map used to store CJS parsing data or for ESM loading. -const cjsSourceCache = new SafeWeakMap(); +const importedCJSCache = new SafeWeakMap(); /** * Map of already-loaded CJS modules to use. */ const cjsExportsCache = new SafeWeakMap(); +const requiredESMSourceCache = new SafeWeakMap(); +const kIsMainSymbol = Symbol('kIsMainSymbol'); +const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader'); +const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol'); +const kIsExecuting = Symbol('kIsExecuting'); // Set first due to cycle with ESM loader functions. module.exports = { cjsExportsCache, - cjsSourceCache, + importedCJSCache, initializeCJS, Module, wrapSafe, + kIsMainSymbol, + kIsCachedByESMLoader, + kRequiredModuleSymbol, + kIsExecuting, }; -const kIsMainSymbol = Symbol('kIsMainSymbol'); - const { BuiltinModule } = require('internal/bootstrap/realm'); const { maybeCacheSourceMap, @@ -137,6 +146,7 @@ const { codes: { ERR_INVALID_ARG_VALUE, ERR_INVALID_MODULE_SPECIFIER, + ERR_REQUIRE_CYCLE_MODULE, ERR_REQUIRE_ESM, ERR_UNKNOWN_BUILTIN_MODULE, }, @@ -942,6 +952,16 @@ const CircularRequirePrototypeWarningProxy = new Proxy({}, { * @param {Module} module The module instance */ function getExportsForCircularRequire(module) { + const requiredESM = module[kRequiredModuleSymbol]; + if (requiredESM && requiredESM.getStatus() !== kEvaluated) { + let message = `Cannot require() ES Module ${module.id} in a cycle.`; + const parent = moduleParentCache.get(module); + if (parent) { + message += ` (from ${parent.filename})`; + } + throw new ERR_REQUIRE_CYCLE_MODULE(message); + } + if (module.exports && !isProxy(module.exports) && ObjectGetPrototypeOf(module.exports) === ObjectPrototype && @@ -1009,11 +1029,21 @@ Module._load = function(request, parent, isMain) { if (cachedModule !== undefined) { updateChildren(parent, cachedModule, true); if (!cachedModule.loaded) { - const parseCachedModule = cjsSourceCache.get(cachedModule); - if (!parseCachedModule || parseCachedModule.loaded) { + // If it's not cached by the ESM loader, the loading request + // comes from required CJS, and we can consider it a circular + // dependency when it's cached. + if (!cachedModule[kIsCachedByESMLoader]) { return getExportsForCircularRequire(cachedModule); } - parseCachedModule.loaded = true; + // If it's cached by the ESM loader as a way to indirectly pass + // the module in to avoid creating it twice, the loading request + // come from imported CJS. In that case use the importedCJSCache + // to determine if it's loading or not. + const importedCJSMetadata = importedCJSCache.get(cachedModule); + if (importedCJSMetadata.loading) { + return getExportsForCircularRequire(cachedModule); + } + importedCJSMetadata.loading = true; } else { return cachedModule.exports; } @@ -1027,18 +1057,21 @@ Module._load = function(request, parent, isMain) { // Don't call updateChildren(), Module constructor already does. const module = cachedModule || new Module(filename, parent); - if (isMain) { - setOwnProperty(process, 'mainModule', module); - setOwnProperty(module.require, 'main', process.mainModule); - module.id = '.'; - module[kIsMainSymbol] = true; - } else { - module[kIsMainSymbol] = false; - } + if (!cachedModule) { + if (isMain) { + setOwnProperty(process, 'mainModule', module); + setOwnProperty(module.require, 'main', process.mainModule); + module.id = '.'; + module[kIsMainSymbol] = true; + } else { + module[kIsMainSymbol] = false; + } - reportModuleToWatchMode(filename); + reportModuleToWatchMode(filename); + Module._cache[filename] = module; + module[kIsCachedByESMLoader] = false; + } - Module._cache[filename] = module; if (parent !== undefined) { relativeResolveCache[relResolveCacheIdentifier] = filename; } @@ -1280,7 +1313,7 @@ function loadESMFromCJS(mod, filename) { const isMain = mod[kIsMainSymbol]; // TODO(joyeecheung): we may want to invent optional special handling for default exports here. // For now, it's good enough to be identical to what `import()` returns. - mod.exports = cascadedLoader.importSyncForRequire(filename, source, isMain); + mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, moduleParentCache.get(mod)); } /** @@ -1366,7 +1399,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) { // Only modules being require()'d really need to avoid TLA. if (loadAsESM) { // Pass the source into the .mjs extension handler indirectly through the cache. - cjsSourceCache.set(this, { source: content }); + requiredESMSourceCache.set(this, content); loadESMFromCJS(this, filename); return; } @@ -1407,6 +1440,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) { const module = this; if (requireDepth === 0) { statCache = new SafeMap(); } setHasStartedUserCJSExecution(); + this[kIsExecuting] = true; if (inspectorWrapper) { result = inspectorWrapper(compiledWrapper, thisValue, exports, require, module, filename, dirname); @@ -1414,6 +1448,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) { result = ReflectApply(compiledWrapper, thisValue, [exports, require, module, filename, dirname]); } + this[kIsExecuting] = false; if (requireDepth === 0) { statCache = null; } return result; }; @@ -1425,7 +1460,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) { * @returns {string} */ function getMaybeCachedSource(mod, filename) { - const cached = cjsSourceCache.get(mod); + const cached = importedCJSCache.get(mod); let content; if (cached?.source) { content = cached.source; @@ -1433,7 +1468,7 @@ function getMaybeCachedSource(mod, filename) { } else { // TODO(joyeecheung): we can read a buffer instead to speed up // compilation. - content = fs.readFileSync(filename, 'utf8'); + content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8'); } return content; } diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js index 5239bc8ed883a5..7b77af35a1dfeb 100644 --- a/lib/internal/modules/esm/load.js +++ b/lib/internal/modules/esm/load.js @@ -152,6 +152,11 @@ async function defaultLoad(url, context = kEmptyObject) { validateAttributes(url, format, importAttributes); + // Use the synchronous commonjs translator which can deal with cycles. + if (format === 'commonjs' && getOptionValue('--experimental-require-module')) { + format = 'commonjs-sync'; + } + return { __proto__: null, format, @@ -201,6 +206,11 @@ function defaultLoadSync(url, context = kEmptyObject) { validateAttributes(url, format, importAttributes); + // Use the synchronous commonjs translator which can deal with cycles. + if (format === 'commonjs' && getOptionValue('--experimental-require-module')) { + format = 'commonjs-sync'; + } + return { __proto__: null, format, diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d39bd37d801d42..4c0b351aa8163e 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -1,7 +1,10 @@ 'use strict'; // This is needed to avoid cycles in esm/resolve <-> cjs/loader -require('internal/modules/cjs/loader'); +const { + kIsExecuting, + kRequiredModuleSymbol, +} = require('internal/modules/cjs/loader'); const { ArrayPrototypeJoin, @@ -15,8 +18,11 @@ const { hardenRegExp, } = primordials; +const { imported_cjs_symbol } = internalBinding('symbols'); + const assert = require('internal/assert'); const { + ERR_REQUIRE_CYCLE_MODULE, ERR_REQUIRE_ESM, ERR_NETWORK_IMPORT_DISALLOWED, ERR_UNKNOWN_MODULE_FORMAT, @@ -30,7 +36,10 @@ const { } = require('internal/modules/esm/utils'); const { kImplicitAssertType } = require('internal/modules/esm/assert'); const { canParse } = internalBinding('url'); -const { ModuleWrap } = internalBinding('module_wrap'); +const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap'); +const { + urlToFilename, +} = require('internal/modules/helpers'); let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; /** @@ -248,17 +257,36 @@ class ModuleLoader { /** * This constructs (creates, instantiates and evaluates) a module graph that * is require()'d. + * @param {import('../cjs/loader.js').Module} mod CJS module wrapper of the ESM. * @param {string} filename Resolved filename of the module being require()'d * @param {string} source Source code. TODO(joyeecheung): pass the raw buffer. * @param {string} isMain Whether this module is a main module. - * @returns {ModuleNamespaceObject} + * @param {import('../cjs/loader.js').Module|undefined} parent Parent module, if any. + * @returns {{ModuleWrap}} */ - importSyncForRequire(filename, source, isMain) { + importSyncForRequire(mod, filename, source, isMain, parent) { const url = pathToFileURL(filename).href; let job = this.loadCache.get(url, kImplicitAssertType); - // This module is already loaded, check whether it's synchronous and return the - // namespace. + // This module job is already created: + // 1. If it was loaded by `require()` before, at this point the instantiation + // is already completed and we can check the whether it is in a cycle + // (in that case the module status is kEvaluaing), and whether the + // required graph is synchronous. + // 2. If it was loaded by `import` before, only allow it if it's already evaluated + // to forbid cycles. + // TODO(joyeecheung): ensure that imported synchronous graphs are evaluated + // synchronously so that any previously imported synchronous graph is already + // evaluated at this point. if (job !== undefined) { + mod[kRequiredModuleSymbol] = job.module; + if (job.module.getStatus() !== kEvaluated) { + const parentFilename = urlToFilename(parent?.filename); + let message = `Cannot require() ES Module ${filename} in a cycle.`; + if (parentFilename) { + message += ` (from ${parentFilename})`; + } + throw new ERR_REQUIRE_CYCLE_MODULE(message); + } return job.module.getNamespaceSync(); } // TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the @@ -270,6 +298,7 @@ class ModuleLoader { const { ModuleJobSync } = require('internal/modules/esm/module_job'); job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk); this.loadCache.set(url, kImplicitAssertType, job); + mod[kRequiredModuleSymbol] = job.module; return job.runSync().namespace; } @@ -304,19 +333,29 @@ class ModuleLoader { const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes; let job = this.loadCache.get(url, resolvedImportAttributes.type); if (job !== undefined) { - // This module is previously imported before. We will return the module now and check - // asynchronicity of the entire graph later, after the graph is instantiated. + // This module is being evaluated, which means it's imported in a previous link + // in a cycle. + if (job.module.getStatus() === kEvaluating) { + const parentFilename = urlToFilename(parentURL); + let message = `Cannot import Module ${specifier} in a cycle.`; + if (parentFilename) { + message += ` (from ${parentFilename})`; + } + throw new ERR_REQUIRE_CYCLE_MODULE(message); + } + // Othersie the module could be imported before but the evaluation may be already + // completed (e.g. the require call is lazy) so it's okay. We will return the + // module now and check asynchronicity of the entire graph later, after the + // graph is instantiated. return job.module; } defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; const loadResult = defaultLoadSync(url, { format, importAttributes }); const { responseURL, source } = loadResult; - let { format: finalFormat } = loadResult; + const { format: finalFormat } = loadResult; this.validateLoadResult(url, finalFormat); - if (finalFormat === 'commonjs') { - finalFormat = 'commonjs-sync'; - } else if (finalFormat === 'wasm') { + if (finalFormat === 'wasm') { assert.fail('WASM is currently unsupported by require(esm)'); } @@ -333,6 +372,20 @@ class ModuleLoader { process.send({ 'watch:import': [url] }); } + const cjsModule = wrap[imported_cjs_symbol]; + if (cjsModule) { + assert(finalFormat === 'commonjs-sync'); + // Check if the ESM initiating import CJS is being required by the same CJS module. + if (cjsModule && cjsModule[kIsExecuting]) { + const parentFilename = urlToFilename(parentURL); + let message = `Cannot import CommonJS Module ${specifier} in a cycle.`; + if (parentFilename) { + message += ` (from ${parentFilename})`; + } + throw new ERR_REQUIRE_CYCLE_MODULE(message); + } + } + const inspectBrk = (isMain && getOptionValue('--inspect-brk')); const { ModuleJobSync } = require('internal/modules/esm/module_job'); job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 4614b37570c65b..7cb4350b675ba9 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -43,9 +43,10 @@ const { stripBOM, } = require('internal/modules/helpers'); const { - Module: CJSModule, - cjsSourceCache, cjsExportsCache, + importedCJSCache, + kIsCachedByESMLoader, + Module: CJSModule, } = require('internal/modules/cjs/loader'); const { fileURLToPath, pathToFileURL, URL } = require('internal/url'); let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { @@ -305,8 +306,7 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) { this.setExport(exportName, value); } this.setExport('default', exports); - }); - + }, module); } translators.set('commonjs-sync', function requireCommonJS(url, source, isMain) { @@ -315,7 +315,7 @@ translators.set('commonjs-sync', function requireCommonJS(url, source, isMain) { return createCJSModuleWrap(url, source, isMain, (module, source, url, filename) => { assert(module === CJSModule._cache[filename]); - CJSModule._load(filename, null, false); + CJSModule._load(filename); }); }); @@ -367,7 +367,7 @@ function cjsPreparseModuleExports(filename, source) { // TODO: Do we want to keep hitting the user mutable CJS loader here? let module = CJSModule._cache[filename]; if (module) { - const cached = cjsSourceCache.get(module); + const cached = importedCJSCache.get(module); if (cached) { return { module, exportNames: cached.exportNames }; } @@ -377,6 +377,7 @@ function cjsPreparseModuleExports(filename, source) { module = new CJSModule(filename); module.filename = filename; module.paths = CJSModule._nodeModulePaths(module.path); + module[kIsCachedByESMLoader] = true; CJSModule._cache[filename] = module; } @@ -391,7 +392,7 @@ function cjsPreparseModuleExports(filename, source) { const exportNames = new SafeSet(new SafeArrayIterator(exports)); // Set first for cycles. - cjsSourceCache.set(module, { source, exportNames }); + importedCJSCache.set(module, { source, exportNames }); if (reexports.length) { module.filename = filename; diff --git a/lib/internal/modules/helpers.js b/lib/internal/modules/helpers.js index 98119165c88d47..eb5c552c500164 100644 --- a/lib/internal/modules/helpers.js +++ b/lib/internal/modules/helpers.js @@ -319,6 +319,15 @@ function normalizeReferrerURL(referrerName) { assert.fail('Unreachable code reached by ' + inspect(referrerName)); } +/** + * @param {string|undefined} url URL to convert to filename + */ +function urlToFilename(url) { + if (url && StringPrototypeStartsWith(url, 'file://')) { + return fileURLToPath(url); + } + return url; +} // Whether we have started executing any user-provided CJS code. // This is set right before we call the wrapped CJS code (not after, @@ -367,4 +376,5 @@ module.exports = { setHasStartedUserESMExecution() { _hasStartedUserESMExecution = true; }, + urlToFilename, }; diff --git a/src/env_properties.h b/src/env_properties.h index 70f49314eebfa5..b4251f14e61680 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -37,6 +37,7 @@ V(handle_onclose_symbol, "handle_onclose") \ V(no_message_symbol, "no_message_symbol") \ V(messaging_deserialize_symbol, "messaging_deserialize_symbol") \ + V(imported_cjs_symbol, "imported_cjs_symbol") \ V(messaging_transfer_symbol, "messaging_transfer_symbol") \ V(messaging_clone_symbol, "messaging_clone_symbol") \ V(messaging_transfer_list_symbol, "messaging_transfer_list_symbol") \ diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 2379cdbd27b45b..eea74bed4bb8a9 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -104,8 +104,8 @@ ModuleWrap* ModuleWrap::GetFromModule(Environment* env, // new ModuleWrap(url, context, source, lineOffset, columnOffset, cachedData) // new ModuleWrap(url, context, source, lineOffset, columOffset, -// hostDefinedOption) new ModuleWrap(url, context, exportNames, -// syntheticExecutionFunction) +// hostDefinedOption) +// new ModuleWrap(url, context, exportNames, evaluationCallback[, cjsModule]) void ModuleWrap::New(const FunctionCallbackInfo& args) { CHECK(args.IsConstructCall()); CHECK_GE(args.Length(), 3); @@ -139,7 +139,8 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { PrimitiveArray::New(isolate, HostDefinedOptions::kLength); Local id_symbol; if (synthetic) { - // new ModuleWrap(url, context, exportNames, syntheticExecutionFunction) + // new ModuleWrap(url, context, exportNames, evaluationCallback[, + // cjsModule]) CHECK(args[3]->IsFunction()); } else { // new ModuleWrap(url, context, source, lineOffset, columOffset, cachedData) @@ -252,6 +253,12 @@ void ModuleWrap::New(const FunctionCallbackInfo& args) { return; } + if (synthetic && args[4]->IsObject() && + that->Set(context, realm->isolate_data()->imported_cjs_symbol(), args[4]) + .IsNothing()) { + return; + } + // Use the extras object as an object whose GetCreationContext() will be the // original `context`, since the `Context` itself strictly speaking cannot // be stored in an internal field. @@ -612,14 +619,12 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo& args) { case v8::Module::Status::kUninstantiated: case v8::Module::Status::kInstantiating: return realm->env()->ThrowError( - "cannot get namespace, module has not been instantiated"); - case v8::Module::Status::kEvaluating: - return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env()); + "Cannot get namespace, module has not been instantiated"); case v8::Module::Status::kInstantiated: case v8::Module::Status::kEvaluated: case v8::Module::Status::kErrored: break; - default: + case v8::Module::Status::kEvaluating: UNREACHABLE(); } diff --git a/test/es-module/test-require-module-cycle-esm-cjs-esm-esm.js b/test/es-module/test-require-module-cycle-esm-cjs-esm-esm.js new file mode 100644 index 00000000000000..3938e03def2408 --- /dev/null +++ b/test/es-module/test-require-module-cycle-esm-cjs-esm-esm.js @@ -0,0 +1,56 @@ +'use strict'; + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +// a.mjs -> b.cjs -> c.mjs -> a.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-esm-cycle/a.mjs'), + ], + { + signal: null, + status: 1, + trim: true, + stderr: /Cannot import Module \.\/a\.mjs in a cycle\. \(from .*c\.mjs\)/, + } + ); +} + +// b.cjs -> c.mjs -> a.mjs -> b.cjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-esm-cycle/b.cjs'), + ], + { + signal: null, + status: 1, + trim: true, + stderr: /Cannot import CommonJS Module \.\/b\.cjs in a cycle\. \(from .*a\.mjs\)/, + } + ); +} + +// c.mjs -> a.mjs -> b.cjs -> c.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-esm-cycle/c.mjs'), + ], + { + signal: null, + status: 1, + trim: true, + stderr: /Cannot require\(\) ES Module .*c\.mjs in a cycle\. \(from .*b\.cjs\)/, + } + ); +} diff --git a/test/es-module/test-require-module-cycle-esm-cjs-esm.js b/test/es-module/test-require-module-cycle-esm-cjs-esm.js new file mode 100644 index 00000000000000..e996f5ed6c70e2 --- /dev/null +++ b/test/es-module/test-require-module-cycle-esm-cjs-esm.js @@ -0,0 +1,72 @@ +'use strict'; + +require('../common'); +const { spawnSyncAndExit, spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +// require-a.cjs -> a.mjs -> b.cjs -> a.mjs. +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-cycle/require-a.cjs'), + ], + { + signal: null, + status: 1, + trim: true, + stderr: /Cannot require\(\) ES Module .*a\.mjs in a cycle\. \(from .*require-a\.cjs\)/, + } + ); +} + +// require-b.cjs -> b.cjs -> a.mjs -> b.cjs. +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-cycle/require-b.cjs'), + ], + { + signal: null, + status: 1, + trim: true, + stderr: /Cannot import CommonJS Module \.\/b\.cjs in a cycle\. \(from .*a\.mjs\)/, + } + ); +} + +// a.mjs -> b.cjs -> a.mjs +{ + spawnSyncAndExit( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-cycle/a.mjs'), + ], + { + signal: null, + status: 1, + stderr: /Cannot require\(\) ES Module .*a\.mjs in a cycle\. \(from .*b\.cjs\)/, + } + ); +} + +// b.cjs -> a.mjs -> b.cjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-cjs-esm-cycle/b.cjs'), + ], + { + signal: null, + status: 1, + trim: true, + stderr: /Cannot import CommonJS Module \.\/b\.cjs in a cycle\. \(from .*a\.mjs\)/, + } + ); +} diff --git a/test/es-module/test-require-module-cycle-esm-esm-cjs-esm-esm.js b/test/es-module/test-require-module-cycle-esm-esm-cjs-esm-esm.js new file mode 100644 index 00000000000000..b4e0cfba807f98 --- /dev/null +++ b/test/es-module/test-require-module-cycle-esm-esm-cjs-esm-esm.js @@ -0,0 +1,70 @@ +'use strict'; + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +// a.mjs -> b.mjs -> c.cjs -> z.mjs -> a.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-esm-cycle/a.mjs'), + ], + { + signal: null, + status: 1, + stderr: /Cannot import Module \.\/a\.mjs in a cycle\. \(from .*z\.mjs\)/, + } + ); +} + +// b.mjs -> c.cjs -> z.mjs -> a.mjs -> b.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-esm-cycle/b.mjs'), + ], + { + signal: null, + status: 1, + stderr: /Cannot import Module \.\/b\.mjs in a cycle\. \(from .*a\.mjs\)/, + } + ); +} + +// c.cjs -> z.mjs -> a.mjs -> b.mjs -> c.cjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-esm-cycle/c.cjs'), + ], + { + signal: null, + status: 1, + stderr: /Cannot import CommonJS Module \.\/c\.cjs in a cycle\. \(from .*b\.mjs\)/, + } + ); +} + + +// z.mjs -> a.mjs -> b.mjs -> c.cjs -> z.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-esm-cycle/z.mjs'), + ], + { + signal: null, + status: 1, + stderr: /Cannot require\(\) ES Module .*z\.mjs in a cycle\. \(from .*c\.cjs\)/, + } + ); +} diff --git a/test/es-module/test-require-module-cycle-esm-esm-cjs-esm.js b/test/es-module/test-require-module-cycle-esm-esm-cjs-esm.js new file mode 100644 index 00000000000000..edec79e276d21c --- /dev/null +++ b/test/es-module/test-require-module-cycle-esm-esm-cjs-esm.js @@ -0,0 +1,82 @@ +'use strict'; + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); + +// a.mjs -> b.mjs -> c.mjs -> d.mjs -> c.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-cycle/a.mjs'), + ], + { + signal: null, + status: 0, + trim: true, + stdout(output) { + assert.match(output, /Start c/); + assert.match(output, /dynamic import b\.mjs failed.*ERR_REQUIRE_CYCLE_MODULE/); + assert.match(output, /dynamic import d\.mjs failed.*ERR_REQUIRE_CYCLE_MODULE/); + } + } + ); +} + +// b.mjs -> c.mjs -> d.mjs -> c.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-cycle/b.mjs'), + ], + { + signal: null, + status: 1, + trim: true, + stdout: /Start c/, + stderr: /Cannot import Module \.\/c\.mjs in a cycle\. \(from .*d\.mjs\)/, + } + ); +} + +// c.mjs -> d.mjs -> c.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-cycle/c.mjs'), + ], + { + signal: null, + status: 1, + trim: true, + stdout: /Start c/, + stderr: /Cannot import Module \.\/c\.mjs in a cycle\. \(from .*d\.mjs\)/, + } + ); +} + + +// d.mjs -> c.mjs -> d.mjs +{ + spawnSyncAndAssert( + process.execPath, + [ + '--experimental-require-module', + fixtures.path('es-modules/esm-esm-cjs-esm-cycle/d.mjs'), + ], + { + signal: null, + status: 1, + trim: true, + stdout: /Start c/, + stderr: /Cannot require\(\) ES Module .*d\.mjs in a cycle\. \(from .*c\.mjs\)/, + } + ); +} diff --git a/test/fixtures/es-modules/esm-cjs-esm-cycle/a.mjs b/test/fixtures/es-modules/esm-cjs-esm-cycle/a.mjs new file mode 100644 index 00000000000000..c4cd38f4c291d9 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-cycle/a.mjs @@ -0,0 +1,3 @@ +import result from './b.cjs'; +export default 'hello'; +console.log('import b.cjs from a.mjs', result); diff --git a/test/fixtures/es-modules/esm-cjs-esm-cycle/b.cjs b/test/fixtures/es-modules/esm-cjs-esm-cycle/b.cjs new file mode 100644 index 00000000000000..4b13c6f7b41093 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-cycle/b.cjs @@ -0,0 +1,3 @@ +const result = require('./a.mjs'); +module.exports = result; +console.log('require a.mjs in b.cjs', result.default); diff --git a/test/fixtures/es-modules/esm-cjs-esm-cycle/require-a.cjs b/test/fixtures/es-modules/esm-cjs-esm-cycle/require-a.cjs new file mode 100644 index 00000000000000..0c434d5c8cefd1 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-cycle/require-a.cjs @@ -0,0 +1 @@ +require('./a.mjs'); diff --git a/test/fixtures/es-modules/esm-cjs-esm-cycle/require-b.cjs b/test/fixtures/es-modules/esm-cjs-esm-cycle/require-b.cjs new file mode 100644 index 00000000000000..802edc44b6ff81 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-cycle/require-b.cjs @@ -0,0 +1 @@ +require('./b.cjs'); diff --git a/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/a.mjs b/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/a.mjs new file mode 100644 index 00000000000000..70ea3568452283 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/a.mjs @@ -0,0 +1 @@ +import './b.cjs' diff --git a/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/b.cjs b/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/b.cjs new file mode 100644 index 00000000000000..c25c0a54dee643 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/b.cjs @@ -0,0 +1 @@ +require('./c.mjs') diff --git a/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/c.mjs b/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/c.mjs new file mode 100644 index 00000000000000..aa1738182a4689 --- /dev/null +++ b/test/fixtures/es-modules/esm-cjs-esm-esm-cycle/c.mjs @@ -0,0 +1 @@ +import './a.mjs' diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/a.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/a.mjs new file mode 100644 index 00000000000000..d9afd0e0adafa0 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/a.mjs @@ -0,0 +1,15 @@ +// a.mjs + +try { + await import('./b.mjs'); + console.log('dynamic import b.mjs did not fail'); +} catch (err) { + console.log('dynamic import b.mjs failed', err); +} + +try { + await import('./d.mjs'); + console.log('dynamic import d.mjs did not fail'); +} catch (err) { + console.log('dynamic import d.mjs failed', err); +} diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/b.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/b.mjs new file mode 100644 index 00000000000000..c9f385a998f5d4 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/b.mjs @@ -0,0 +1,3 @@ +// b.mjs +import "./c.mjs"; +console.log("Execute b"); diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/c.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/c.mjs new file mode 100644 index 00000000000000..2be14ac85fa9fe --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/c.mjs @@ -0,0 +1,5 @@ +// c.mjs +import { createRequire } from "module"; +console.log("Start c"); +createRequire(import.meta.url)("./d.mjs"); +throw new Error("Error from c"); diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/d.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/d.mjs new file mode 100644 index 00000000000000..90cdffa0ad72e5 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-cycle/d.mjs @@ -0,0 +1,3 @@ +// d.mjs +import "./c.mjs"; +console.log("Execute d"); diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/a.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/a.mjs new file mode 100644 index 00000000000000..a197977dfe0672 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/a.mjs @@ -0,0 +1 @@ +import './b.mjs' diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/b.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/b.mjs new file mode 100644 index 00000000000000..e3522f015744d0 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/b.mjs @@ -0,0 +1 @@ +import './c.cjs' diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/c.cjs b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/c.cjs new file mode 100644 index 00000000000000..6d6a2a0fa8dbb4 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/c.cjs @@ -0,0 +1 @@ +require('./z.mjs') diff --git a/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/z.mjs b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/z.mjs new file mode 100644 index 00000000000000..aa1738182a4689 --- /dev/null +++ b/test/fixtures/es-modules/esm-esm-cjs-esm-esm-cycle/z.mjs @@ -0,0 +1 @@ +import './a.mjs' From 99c718f43f889071ad6f0aaa66b4c0c3940505e7 Mon Sep 17 00:00:00 2001 From: Chengzhong Wu Date: Fri, 12 Apr 2024 15:39:56 +0100 Subject: [PATCH 4/6] lib: convert WeakMaps in cjs loader with private symbol properties Symbol properties are typically more GC-efficient than using WeakMaps, since WeakMap requires ephemeron GC. `module[kModuleExportNames]` would be easier to read than `importedCJSCache.get(module).exportNames` as well. PR-URL: https://github.com/nodejs/node/pull/52095 Reviewed-By: Geoffrey Booth Reviewed-By: Joyee Cheung Reviewed-By: Antoine du Hamel --- lib/internal/modules/cjs/loader.js | 107 ++++++++++++++---------- lib/internal/modules/esm/translators.js | 21 +++-- src/env_properties.h | 5 ++ 3 files changed, 79 insertions(+), 54 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 1b5dc834953534..042f4d061c2c01 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -50,7 +50,6 @@ const { ReflectSet, RegExpPrototypeExec, SafeMap, - SafeWeakMap, String, StringPrototypeCharAt, StringPrototypeCharCodeAt, @@ -62,16 +61,39 @@ const { StringPrototypeStartsWith, Symbol, } = primordials; +const { + privateSymbols: { + module_source_private_symbol, + module_export_names_private_symbol, + module_circular_visited_private_symbol, + module_export_private_symbol, + module_parent_private_symbol, + }, +} = internalBinding('util'); const { kEvaluated } = internalBinding('module_wrap'); -// Map used to store CJS parsing data or for ESM loading. -const importedCJSCache = new SafeWeakMap(); +// Internal properties for Module instances. +/** + * Cached {@link Module} source string. + */ +const kModuleSource = module_source_private_symbol; +/** + * Cached {@link Module} export names for ESM loader. + */ +const kModuleExportNames = module_export_names_private_symbol; +/** + * {@link Module} circular dependency visited flag. + */ +const kModuleCircularVisited = module_circular_visited_private_symbol; /** - * Map of already-loaded CJS modules to use. + * {@link Module} export object snapshot for ESM loader. */ -const cjsExportsCache = new SafeWeakMap(); -const requiredESMSourceCache = new SafeWeakMap(); +const kModuleExport = module_export_private_symbol; +/** + * {@link Module} parent module. + */ +const kModuleParent = module_parent_private_symbol; const kIsMainSymbol = Symbol('kIsMainSymbol'); const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader'); @@ -79,8 +101,10 @@ const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol'); const kIsExecuting = Symbol('kIsExecuting'); // Set first due to cycle with ESM loader functions. module.exports = { - cjsExportsCache, - importedCJSCache, + kModuleSource, + kModuleExport, + kModuleExportNames, + kModuleCircularVisited, initializeCJS, Module, wrapSafe, @@ -256,8 +280,6 @@ function reportModuleNotFoundToWatchMode(basePath, extensions) { } } -/** @type {Map} */ -const moduleParentCache = new SafeWeakMap(); /** * Create a new module instance. * @param {string} id @@ -267,7 +289,7 @@ function Module(id = '', parent) { this.id = id; this.path = path.dirname(id); setOwnProperty(this, 'exports', {}); - moduleParentCache.set(this, parent); + this[kModuleParent] = parent; updateChildren(parent, this, false); this.filename = null; this.loaded = false; @@ -355,17 +377,19 @@ ObjectDefineProperty(BuiltinModule.prototype, 'isPreloading', isPreloadingDesc); /** * Get the parent of the current module from our cache. + * @this {Module} */ function getModuleParent() { - return moduleParentCache.get(this); + return this[kModuleParent]; } /** * Set the parent of the current module in our cache. + * @this {Module} * @param {Module} value */ function setModuleParent(value) { - moduleParentCache.set(this, value); + this[kModuleParent] = value; } let debug = require('internal/util/debuglog').debuglog('module', (fn) => { @@ -955,7 +979,7 @@ function getExportsForCircularRequire(module) { const requiredESM = module[kRequiredModuleSymbol]; if (requiredESM && requiredESM.getStatus() !== kEvaluated) { let message = `Cannot require() ES Module ${module.id} in a cycle.`; - const parent = moduleParentCache.get(module); + const parent = module[kModuleParent]; if (parent) { message += ` (from ${parent.filename})`; } @@ -1028,25 +1052,24 @@ Module._load = function(request, parent, isMain) { const cachedModule = Module._cache[filename]; if (cachedModule !== undefined) { updateChildren(parent, cachedModule, true); - if (!cachedModule.loaded) { - // If it's not cached by the ESM loader, the loading request - // comes from required CJS, and we can consider it a circular - // dependency when it's cached. - if (!cachedModule[kIsCachedByESMLoader]) { - return getExportsForCircularRequire(cachedModule); - } - // If it's cached by the ESM loader as a way to indirectly pass - // the module in to avoid creating it twice, the loading request - // come from imported CJS. In that case use the importedCJSCache - // to determine if it's loading or not. - const importedCJSMetadata = importedCJSCache.get(cachedModule); - if (importedCJSMetadata.loading) { - return getExportsForCircularRequire(cachedModule); - } - importedCJSMetadata.loading = true; - } else { + if (cachedModule.loaded) { return cachedModule.exports; } + // If it's not cached by the ESM loader, the loading request + // comes from required CJS, and we can consider it a circular + // dependency when it's cached. + if (!cachedModule[kIsCachedByESMLoader]) { + return getExportsForCircularRequire(cachedModule); + } + // If it's cached by the ESM loader as a way to indirectly pass + // the module in to avoid creating it twice, the loading request + // come from imported CJS. In that case use the kModuleCircularVisited + // to determine if it's loading or not. + if (cachedModule[kModuleCircularVisited]) { + return getExportsForCircularRequire(cachedModule); + } + // This is an ESM loader created cache entry, mark it as visited and fallthrough to loading the module. + cachedModule[kModuleCircularVisited] = true; } if (BuiltinModule.canBeRequiredWithoutScheme(filename)) { @@ -1190,7 +1213,7 @@ Module._resolveFilename = function(request, parent, isMain, options) { const requireStack = []; for (let cursor = parent; cursor; - cursor = moduleParentCache.get(cursor)) { + cursor = cursor[kModuleParent]) { ArrayPrototypePush(requireStack, cursor.filename || cursor.id); } let message = `Cannot find module '${request}'`; @@ -1268,9 +1291,7 @@ Module.prototype.load = function(filename) { // Create module entry at load time to snapshot exports correctly const exports = this.exports; // Preemptively cache for ESM loader. - if (!cjsExportsCache.has(this)) { - cjsExportsCache.set(this, exports); - } + this[kModuleExport] = exports; }; /** @@ -1313,7 +1334,7 @@ function loadESMFromCJS(mod, filename) { const isMain = mod[kIsMainSymbol]; // TODO(joyeecheung): we may want to invent optional special handling for default exports here. // For now, it's good enough to be identical to what `import()` returns. - mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, moduleParentCache.get(mod)); + mod.exports = cascadedLoader.importSyncForRequire(mod, filename, source, isMain, mod[kModuleParent]); } /** @@ -1399,7 +1420,7 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) { // Only modules being require()'d really need to avoid TLA. if (loadAsESM) { // Pass the source into the .mjs extension handler indirectly through the cache. - requiredESMSourceCache.set(this, content); + this[kModuleSource] = content; loadESMFromCJS(this, filename); return; } @@ -1460,15 +1481,15 @@ Module.prototype._compile = function(content, filename, loadAsESM = false) { * @returns {string} */ function getMaybeCachedSource(mod, filename) { - const cached = importedCJSCache.get(mod); + // If already analyzed the source, then it will be cached. let content; - if (cached?.source) { - content = cached.source; - cached.source = undefined; + if (mod[kModuleSource] !== undefined) { + content = mod[kModuleSource]; + mod[kModuleSource] = undefined; } else { // TODO(joyeecheung): we can read a buffer instead to speed up // compilation. - content = requiredESMSourceCache.get(mod) ?? fs.readFileSync(filename, 'utf8'); + content = fs.readFileSync(filename, 'utf8'); } return content; } @@ -1492,7 +1513,7 @@ Module._extensions['.js'] = function(module, filename) { } // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed. - const parent = moduleParentCache.get(module); + const parent = module[kModuleParent]; const parentPath = parent?.filename; const packageJsonPath = path.resolve(pkg.path, 'package.json'); const usesEsm = containsModuleSyntax(content, filename); diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 7cb4350b675ba9..4bdee1b843fbd5 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -43,10 +43,11 @@ const { stripBOM, } = require('internal/modules/helpers'); const { - cjsExportsCache, - importedCJSCache, kIsCachedByESMLoader, Module: CJSModule, + kModuleSource, + kModuleExport, + kModuleExportNames, } = require('internal/modules/cjs/loader'); const { fileURLToPath, pathToFileURL, URL } = require('internal/url'); let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { @@ -285,9 +286,9 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) { } let exports; - if (cjsExportsCache.has(module)) { - exports = cjsExportsCache.get(module); - cjsExportsCache.delete(module); + if (module[kModuleExport] !== undefined) { + exports = module[kModuleExport]; + module[kModuleExport] = undefined; } else { ({ exports } = module); } @@ -366,11 +367,8 @@ translators.set('commonjs', async function commonjsStrategy(url, source, function cjsPreparseModuleExports(filename, source) { // TODO: Do we want to keep hitting the user mutable CJS loader here? let module = CJSModule._cache[filename]; - if (module) { - const cached = importedCJSCache.get(module); - if (cached) { - return { module, exportNames: cached.exportNames }; - } + if (module && module[kModuleExportNames] !== undefined) { + return { module, exportNames: module[kModuleExportNames] }; } const loaded = Boolean(module); if (!loaded) { @@ -378,6 +376,7 @@ function cjsPreparseModuleExports(filename, source) { module.filename = filename; module.paths = CJSModule._nodeModulePaths(module.path); module[kIsCachedByESMLoader] = true; + module[kModuleSource] = source; CJSModule._cache[filename] = module; } @@ -392,7 +391,7 @@ function cjsPreparseModuleExports(filename, source) { const exportNames = new SafeSet(new SafeArrayIterator(exports)); // Set first for cycles. - importedCJSCache.set(module, { source, exportNames }); + module[kModuleExportNames] = exportNames; if (reexports.length) { module.filename = filename; diff --git a/src/env_properties.h b/src/env_properties.h index b4251f14e61680..32ccbce46245a3 100644 --- a/src/env_properties.h +++ b/src/env_properties.h @@ -22,6 +22,11 @@ V(contextify_context_private_symbol, "node:contextify:context") \ V(decorated_private_symbol, "node:decorated") \ V(host_defined_option_symbol, "node:host_defined_option_symbol") \ + V(module_source_private_symbol, "node:module_source") \ + V(module_export_names_private_symbol, "node:module_export_names") \ + V(module_circular_visited_private_symbol, "node:module_circular_visited") \ + V(module_export_private_symbol, "node:module_export") \ + V(module_parent_private_symbol, "node:module_parent") \ V(napi_type_tag, "node:napi:type_tag") \ V(napi_wrapper, "node:napi:wrapper") \ V(untransferable_object_private_symbol, "node:untransferableObject") \ From e2583038230f64e3ee5fdc7aebeaf1217385176c Mon Sep 17 00:00:00 2001 From: Jacob Smith <3012099+JakobJingleheimer@users.noreply.github.com> Date: Sat, 13 Apr 2024 21:41:20 +0200 Subject: [PATCH 5/6] module: tidy code and comments PR-URL: https://github.com/nodejs/node/pull/52437 Reviewed-By: Geoffrey Booth Reviewed-By: Yagiz Nizipli Reviewed-By: Joyee Cheung Reviewed-By: Luigi Pinca Reviewed-By: Moshe Atlow Reviewed-By: Antoine du Hamel Reviewed-By: Feng Yu --- doc/api/errors.md | 2 +- lib/internal/modules/cjs/loader.js | 2 +- lib/internal/modules/esm/loader.js | 28 ++++++++++++++++++------- lib/internal/modules/esm/translators.js | 5 +++-- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/doc/api/errors.md b/doc/api/errors.md index 7a1d6d12b981c9..eaefe13560ccba 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -2533,7 +2533,7 @@ This is not allowed because ES Modules cannot be evaluated while they are already being evaluated. To avoid the cycle, the `require()` call involved in a cycle should not happen -at the top-level of either a ES Module (via `createRequire()`) or a CommonJS +at the top-level of either an ES Module (via `createRequire()`) or a CommonJS module, and should be done lazily in an inner function. diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 042f4d061c2c01..c284b39b1ac13e 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -1063,7 +1063,7 @@ Module._load = function(request, parent, isMain) { } // If it's cached by the ESM loader as a way to indirectly pass // the module in to avoid creating it twice, the loading request - // come from imported CJS. In that case use the kModuleCircularVisited + // came from imported CJS. In that case use the kModuleCircularVisited // to determine if it's loading or not. if (cachedModule[kModuleCircularVisited]) { return getExportsForCircularRequire(cachedModule); diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 4c0b351aa8163e..418bbbfe7bdf39 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -42,6 +42,10 @@ const { } = require('internal/modules/helpers'); let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; +/** + * @typedef {import('url').URL} URL + */ + /** * Lazy loads the module_map module and returns a new instance of ResolveCache. * @returns {import('./module_map.js').ResolveCache} @@ -77,6 +81,10 @@ function getTranslators() { */ let hooksProxy; +/** + * @typedef {import('../cjs/loader.js').Module} CJSModule + */ + /** * @typedef {Record} ModuleExports */ @@ -257,11 +265,11 @@ class ModuleLoader { /** * This constructs (creates, instantiates and evaluates) a module graph that * is require()'d. - * @param {import('../cjs/loader.js').Module} mod CJS module wrapper of the ESM. + * @param {CJSModule} mod CJS module wrapper of the ESM. * @param {string} filename Resolved filename of the module being require()'d * @param {string} source Source code. TODO(joyeecheung): pass the raw buffer. * @param {string} isMain Whether this module is a main module. - * @param {import('../cjs/loader.js').Module|undefined} parent Parent module, if any. + * @param {CJSModule|undefined} parent Parent module, if any. * @returns {{ModuleWrap}} */ importSyncForRequire(mod, filename, source, isMain, parent) { @@ -343,7 +351,7 @@ class ModuleLoader { } throw new ERR_REQUIRE_CYCLE_MODULE(message); } - // Othersie the module could be imported before but the evaluation may be already + // Otherwise the module could be imported before but the evaluation may be already // completed (e.g. the require call is lazy) so it's okay. We will return the // module now and check asynchronicity of the entire graph later, after the // graph is instantiated. @@ -352,8 +360,12 @@ class ModuleLoader { defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; const loadResult = defaultLoadSync(url, { format, importAttributes }); - const { responseURL, source } = loadResult; - const { format: finalFormat } = loadResult; + const { + format: finalFormat, + responseURL, + source, + } = loadResult; + this.validateLoadResult(url, finalFormat); if (finalFormat === 'wasm') { assert.fail('WASM is currently unsupported by require(esm)'); @@ -725,11 +737,11 @@ function getOrInitializeCascadedLoader() { /** * Register a single loader programmatically. - * @param {string|import('url').URL} specifier - * @param {string|import('url').URL} [parentURL] Base to use when resolving `specifier`; optional if + * @param {string|URL} specifier + * @param {string|URL} [parentURL] Base to use when resolving `specifier`; optional if * `specifier` is absolute. Same as `options.parentUrl`, just inline * @param {object} [options] Additional options to apply, described below. - * @param {string|import('url').URL} [options.parentURL] Base to use when resolving `specifier` + * @param {string|URL} [options.parentURL] Base to use when resolving `specifier` * @param {any} [options.data] Arbitrary data passed to the loader's `initialize` hook * @param {any[]} [options.transferList] Objects in `data` that are changing ownership * @returns {void} We want to reserve the return value for potential future extension of the API. diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 4bdee1b843fbd5..8f4b6b25d88896 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -41,6 +41,7 @@ const { dirname, extname, isAbsolute } = require('path'); const { loadBuiltinModule, stripBOM, + urlToFilename, } = require('internal/modules/helpers'); const { kIsCachedByESMLoader, @@ -243,7 +244,7 @@ function loadCJSModule(module, source, url, filename) { } } const { url: resolvedURL } = cascadedLoader.resolveSync(specifier, url, kEmptyObject); - return StringPrototypeStartsWith(resolvedURL, 'file://') ? fileURLToPath(resolvedURL) : resolvedURL; + return urlToFilename(resolvedURL); }); setOwnProperty(requireFn, 'main', process.mainModule); @@ -265,7 +266,7 @@ const cjsCache = new SafeMap(); function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) { debug(`Translating CJSModule ${url}`); - const filename = StringPrototypeStartsWith(url, 'file://') ? fileURLToPath(url) : url; + const filename = urlToFilename(url); // In case the source was not provided by the `load` step, we need fetch it now. source = stringify(source ?? getSource(new URL(url)).source); From 7298d8dc193b4c9d94337a4688e4bfd96d9f5195 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 16 Apr 2024 16:07:44 +0200 Subject: [PATCH 6/6] module: fix submodules loaded by require() and import() Previously there is an edge case where submodules loaded by require() may not be loaded by import() again from different intermediate edges in the graph. This patch fixes that, added tests, and added debug logs. Drive-by: make loader a private field so it doesn't show up in logs. PR-URL: https://github.com/nodejs/node/pull/52487 Reviewed-By: Geoffrey Booth Reviewed-By: Benjamin Gruenbaum --- lib/internal/modules/esm/loader.js | 6 ++-- lib/internal/modules/esm/module_job.js | 34 ++++++++++++++----- .../test-require-module-dynamic-import-3.js | 14 ++++++++ .../test-require-module-dynamic-import-4.js | 14 ++++++++ .../es-modules/require-and-import/load.cjs | 2 ++ .../es-modules/require-and-import/load.mjs | 2 ++ .../node_modules/dep/mod.js | 2 ++ .../node_modules/dep/package.json | 5 +++ 8 files changed, 67 insertions(+), 12 deletions(-) create mode 100644 test/es-module/test-require-module-dynamic-import-3.js create mode 100644 test/es-module/test-require-module-dynamic-import-4.js create mode 100644 test/fixtures/es-modules/require-and-import/load.cjs create mode 100644 test/fixtures/es-modules/require-and-import/load.mjs create mode 100644 test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js create mode 100644 test/fixtures/es-modules/require-and-import/node_modules/dep/package.json diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 418bbbfe7bdf39..4b88ed67703f66 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -316,7 +316,7 @@ class ModuleLoader { * @param {string} specifier Specifier of the the imported module. * @param {string} parentURL Where the import comes from. * @param {object} importAttributes import attributes from the import statement. - * @returns {ModuleWrap} + * @returns {ModuleJobBase} */ getModuleWrapForRequire(specifier, parentURL, importAttributes) { assert(getOptionValue('--experimental-require-module')); @@ -355,7 +355,7 @@ class ModuleLoader { // completed (e.g. the require call is lazy) so it's okay. We will return the // module now and check asynchronicity of the entire graph later, after the // graph is instantiated. - return job.module; + return job; } defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; @@ -403,7 +403,7 @@ class ModuleLoader { job = new ModuleJobSync(this, url, importAttributes, wrap, isMain, inspectBrk); this.loadCache.set(url, importAttributes.type, job); - return job.module; + return job; } /** diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index a9e0f72579b6f8..4bb6a72c72aa06 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -18,6 +18,9 @@ const { StringPrototypeSplit, StringPrototypeStartsWith, } = primordials; +let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { + debug = fn; +}); const { ModuleWrap, kEvaluated } = internalBinding('module_wrap'); @@ -48,8 +51,7 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) => ); class ModuleJobBase { - constructor(loader, url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) { - this.loader = loader; + constructor(url, importAttributes, moduleWrapMaybePromise, isMain, inspectBrk) { this.importAttributes = importAttributes; this.isMain = isMain; this.inspectBrk = inspectBrk; @@ -62,11 +64,13 @@ class ModuleJobBase { /* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of * its dependencies, over time. */ class ModuleJob extends ModuleJobBase { + #loader = null; // `loader` is the Loader instance used for loading dependencies. constructor(loader, url, importAttributes = { __proto__: null }, moduleProvider, isMain, inspectBrk, sync = false) { const modulePromise = ReflectApply(moduleProvider, loader, [url, isMain]); - super(loader, url, importAttributes, modulePromise, isMain, inspectBrk); + super(url, importAttributes, modulePromise, isMain, inspectBrk); + this.#loader = loader; // Expose the promise to the ModuleWrap directly for linking below. // `this.module` is also filled in below. this.modulePromise = modulePromise; @@ -89,7 +93,8 @@ class ModuleJob extends ModuleJobBase { // these `link` callbacks depending on each other. const dependencyJobs = []; const promises = this.module.link(async (specifier, attributes) => { - const job = await this.loader.getModuleJob(specifier, url, attributes); + const job = await this.#loader.getModuleJob(specifier, url, attributes); + debug(`async link() ${this.url} -> ${specifier}`, job); ArrayPrototypePush(dependencyJobs, job); return job.modulePromise; }); @@ -121,6 +126,8 @@ class ModuleJob extends ModuleJobBase { async _instantiate() { const jobsInGraph = new SafeSet(); const addJobsToDependencyGraph = async (moduleJob) => { + debug(`async addJobsToDependencyGraph() ${this.url}`, moduleJob); + if (jobsInGraph.has(moduleJob)) { return; } @@ -156,7 +163,7 @@ class ModuleJob extends ModuleJobBase { const { 1: childSpecifier, 2: name } = RegExpPrototypeExec( /module '(.*)' does not provide an export named '(.+)'/, e.message); - const { url: childFileURL } = await this.loader.resolve( + const { url: childFileURL } = await this.#loader.resolve( childSpecifier, parentFileUrl, kEmptyObject, @@ -167,7 +174,7 @@ class ModuleJob extends ModuleJobBase { // in the import attributes and some formats require them; but we only // care about CommonJS for the purposes of this error message. ({ format } = - await this.loader.load(childFileURL)); + await this.#loader.load(childFileURL)); } catch { // Continue regardless of error. } @@ -257,18 +264,27 @@ class ModuleJob extends ModuleJobBase { // All the steps are ensured to be synchronous and it throws on instantiating // an asynchronous graph. class ModuleJobSync extends ModuleJobBase { + #loader = null; constructor(loader, url, importAttributes, moduleWrap, isMain, inspectBrk) { - super(loader, url, importAttributes, moduleWrap, isMain, inspectBrk, true); + super(url, importAttributes, moduleWrap, isMain, inspectBrk, true); assert(this.module instanceof ModuleWrap); + this.#loader = loader; const moduleRequests = this.module.getModuleRequestsSync(); + const linked = []; for (let i = 0; i < moduleRequests.length; ++i) { const { 0: specifier, 1: attributes } = moduleRequests[i]; - const wrap = this.loader.getModuleWrapForRequire(specifier, url, attributes); + const job = this.#loader.getModuleWrapForRequire(specifier, url, attributes); const isLast = (i === moduleRequests.length - 1); // TODO(joyeecheung): make the resolution callback deal with both promisified // an raw module wraps, then we don't need to wrap it with a promise here. - this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(wrap), isLast); + this.module.cacheResolvedWrapsSync(specifier, PromiseResolve(job.module), isLast); + ArrayPrototypePush(linked, job); } + this.linked = linked; + } + + get modulePromise() { + return PromiseResolve(this.module); } async run() { diff --git a/test/es-module/test-require-module-dynamic-import-3.js b/test/es-module/test-require-module-dynamic-import-3.js new file mode 100644 index 00000000000000..7a5fbf1a137f96 --- /dev/null +++ b/test/es-module/test-require-module-dynamic-import-3.js @@ -0,0 +1,14 @@ +// Flags: --experimental-require-module +'use strict'; + +// This tests that previously synchronously loaded submodule can still +// be loaded by dynamic import(). + +const common = require('../common'); +const assert = require('assert'); + +(async () => { + const required = require('../fixtures/es-modules/require-and-import/load.cjs'); + const imported = await import('../fixtures/es-modules/require-and-import/load.mjs'); + assert.deepStrictEqual({ ...required }, { ...imported }); +})().then(common.mustCall()); diff --git a/test/es-module/test-require-module-dynamic-import-4.js b/test/es-module/test-require-module-dynamic-import-4.js new file mode 100644 index 00000000000000..414cd70d82d33a --- /dev/null +++ b/test/es-module/test-require-module-dynamic-import-4.js @@ -0,0 +1,14 @@ +// Flags: --experimental-require-module +'use strict'; + +// This tests that previously asynchronously loaded submodule can still +// be loaded by require(). + +const common = require('../common'); +const assert = require('assert'); + +(async () => { + const imported = await import('../fixtures/es-modules/require-and-import/load.mjs'); + const required = require('../fixtures/es-modules/require-and-import/load.cjs'); + assert.deepStrictEqual({ ...required }, { ...imported }); +})().then(common.mustCall()); diff --git a/test/fixtures/es-modules/require-and-import/load.cjs b/test/fixtures/es-modules/require-and-import/load.cjs new file mode 100644 index 00000000000000..ec9c535a04352d --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/load.cjs @@ -0,0 +1,2 @@ +module.exports = require('dep'); + diff --git a/test/fixtures/es-modules/require-and-import/load.mjs b/test/fixtures/es-modules/require-and-import/load.mjs new file mode 100644 index 00000000000000..f5d2135d4dca44 --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/load.mjs @@ -0,0 +1,2 @@ +export * from 'dep'; + diff --git a/test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js b/test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js new file mode 100644 index 00000000000000..0554fa3d5c3bfb --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/node_modules/dep/mod.js @@ -0,0 +1,2 @@ +export const hello = 'world'; + diff --git a/test/fixtures/es-modules/require-and-import/node_modules/dep/package.json b/test/fixtures/es-modules/require-and-import/node_modules/dep/package.json new file mode 100644 index 00000000000000..7aeabf8c45119d --- /dev/null +++ b/test/fixtures/es-modules/require-and-import/node_modules/dep/package.json @@ -0,0 +1,5 @@ +{ + "type": "module", + "main": "mod.js" +} +