From 269b145f7240e3797192bccf662035359b425e67 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 8 Apr 2024 16:45:55 +0200 Subject: [PATCH] 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'