From 9b72f5fc4224900bd214108cb0aa1521d2f6ba78 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 31 Oct 2024 16:43:57 +0100 Subject: [PATCH 1/9] module: handle .mjs in .js handler in CommonJS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This refactors the CommonJS loading a bit to create a center point that handles source loading (`loadSource`) and make format detection more consistent to pave the way for future synchronous hooks. - Handle .mjs in the .js handler, similar to how .cjs has been handled. - Generate the legacy ERR_REQUIRE_ESM in a getRequireESMError() for require(esm) handling (when it's disabled). PR-URL: https://github.com/nodejs/node/pull/55590 Refs: https://github.com/nodejs/loaders/pull/198 Reviewed-By: Antoine du Hamel Reviewed-By: Matteo Collina Reviewed-By: Marco Ippolito Reviewed-By: Juan José Arboleda --- lib/internal/modules/cjs/loader.js | 165 +++++++++++++++-------------- 1 file changed, 86 insertions(+), 79 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index a02aaa07de27ac..b12594cc364056 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -101,6 +101,9 @@ const kIsMainSymbol = Symbol('kIsMainSymbol'); const kIsCachedByESMLoader = Symbol('kIsCachedByESMLoader'); const kRequiredModuleSymbol = Symbol('kRequiredModuleSymbol'); const kIsExecuting = Symbol('kIsExecuting'); + +const kFormat = Symbol('kFormat'); + // Set first due to cycle with ESM loader functions. module.exports = { kModuleSource, @@ -438,10 +441,6 @@ 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')) { - Module._extensions['.mjs'] = loadESMFromCJS; - } } // Given a module name, and a list of paths to test, returns the first @@ -651,14 +650,7 @@ 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); + return extensions; } /** @@ -1270,10 +1262,6 @@ Module.prototype.load = function(filename) { this.paths = Module._nodeModulePaths(path.dirname(filename)); const extension = findLongestRegisteredExtension(filename); - // allow .mjs to be overridden - if (StringPrototypeEndsWith(filename, '.mjs') && !Module._extensions['.mjs']) { - throw new ERR_REQUIRE_ESM(filename, true); - } Module._extensions[extension](this, filename); this.loaded = true; @@ -1309,9 +1297,10 @@ let requireModuleWarningMode; * 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. + * @param {string} format Format of the module. If it had types, this would be what it is after type-stripping. + * @param {string} source Source the module. If it had types, this would have the type stripped. */ -function loadESMFromCJS(mod, filename) { - const source = getMaybeCachedSource(mod, filename); +function loadESMFromCJS(mod, filename, format, source) { const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); const isMain = mod[kIsMainSymbol]; if (isMain) { @@ -1487,7 +1476,9 @@ function wrapSafe(filename, content, cjsModuleInstance, format) { * `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 {'module'|'commonjs'|undefined} format Intended format of the module. + * @param { + * 'module'|'commonjs'|'commonjs-typescript'|'module-typescript' + * } format Intended format of the module. */ Module.prototype._compile = function(content, filename, format) { let moduleURL; @@ -1509,9 +1500,7 @@ Module.prototype._compile = function(content, filename, format) { } if (format === 'module') { - // Pass the source into the .mjs extension handler indirectly through the cache. - this[kModuleSource] = content; - loadESMFromCJS(this, filename); + loadESMFromCJS(this, filename, format, content); return; } @@ -1539,22 +1528,72 @@ Module.prototype._compile = function(content, filename, format) { /** * Get the source code of a module, using cached ones if it's cached. + * After this returns, mod[kFormat], mod[kModuleSource] and mod[kURL] will be set. * @param {Module} mod Module instance whose source is potentially already cached. * @param {string} filename Absolute path to the file of the module. - * @returns {string} + * @returns {{source: string, format?: string}} */ -function getMaybeCachedSource(mod, filename) { - // If already analyzed the source, then it will be cached. - let content; - if (mod[kModuleSource] !== undefined) { - content = mod[kModuleSource]; +function loadSource(mod, filename, formatFromNode) { + if (formatFromNode !== undefined) { + mod[kFormat] = formatFromNode; + } + const format = mod[kFormat]; + + let source = mod[kModuleSource]; + if (source !== undefined) { mod[kModuleSource] = undefined; } else { // TODO(joyeecheung): we can read a buffer instead to speed up // compilation. - content = fs.readFileSync(filename, 'utf8'); + source = fs.readFileSync(filename, 'utf8'); + } + return { source, format }; +} + +function reconstructErrorStack(err, parentPath, parentSource) { + const errLine = StringPrototypeSplit( + StringPrototypeSlice(err.stack, StringPrototypeIndexOf( + err.stack, ' at ')), '\n', 1)[0]; + const { 1: line, 2: col } = + RegExpPrototypeExec(/(\d+):(\d+)\)/, errLine) || []; + if (line && col) { + const srcLine = StringPrototypeSplit(parentSource, '\n')[line - 1]; + const frame = `${parentPath}:${line}\n${srcLine}\n${StringPrototypeRepeat(' ', col - 1)}^\n`; + setArrowMessage(err, frame); + } +} + +/** + * Generate the legacy ERR_REQUIRE_ESM for the cases where require(esm) is disabled. + * @param {Module} mod The module being required. + * @param {undefined|object} pkg Data of the nearest package.json of the module. + * @param {string} content Source code of the module. + * @param {string} filename Filename of the module + * @returns {Error} + */ +function getRequireESMError(mod, pkg, content, filename) { + // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed. + const parent = mod[kModuleParent]; + const parentPath = parent?.filename; + const packageJsonPath = pkg?.path ? path.resolve(pkg.path, 'package.json') : null; + const usesEsm = containsModuleSyntax(content, filename); + const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath, + packageJsonPath); + // Attempt to reconstruct the parent require frame. + const parentModule = Module._cache[parentPath]; + if (parentModule) { + let parentSource; + try { + ({ source: parentSource } = loadSource(parentModule, parentPath)); + } catch { + // Continue regardless of error. + } + if (parentSource) { + // TODO(joyeecheung): trim off internal frames from the stack. + reconstructErrorStack(err, parentPath, parentSource); + } } - return content; + return err; } /** @@ -1563,57 +1602,25 @@ function getMaybeCachedSource(mod, filename) { * @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); - - let format; - 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, 'module'); - return; - } - - // This is an error path because `require` of a `.js` file in a `"type": "module"` scope is not allowed. - const parent = module[kModuleParent]; - const parentPath = parent?.filename; - const packageJsonPath = path.resolve(pkg.path, 'package.json'); - const usesEsm = containsModuleSyntax(content, filename); - const err = new ERR_REQUIRE_ESM(filename, usesEsm, parentPath, - packageJsonPath); - // Attempt to reconstruct the parent require frame. - if (Module._cache[parentPath]) { - let parentSource; - try { - parentSource = fs.readFileSync(parentPath, 'utf8'); - } catch { - // Continue regardless of error. - } - if (parentSource) { - const errLine = StringPrototypeSplit( - StringPrototypeSlice(err.stack, StringPrototypeIndexOf( - err.stack, ' at ')), '\n', 1)[0]; - const { 1: line, 2: col } = - RegExpPrototypeExec(/(\d+):(\d+)\)/, errLine) || []; - if (line && col) { - const srcLine = StringPrototypeSplit(parentSource, '\n')[line - 1]; - const frame = `${parentPath}:${line}\n${srcLine}\n${ - StringPrototypeRepeat(' ', col - 1)}^\n`; - setArrowMessage(err, frame); - } - } - } - throw err; - } else if (pkg.data?.type === 'commonjs') { - format = 'commonjs'; - } - } else if (StringPrototypeEndsWith(filename, '.cjs')) { + let format, pkg; + if (StringPrototypeEndsWith(filename, '.cjs')) { format = 'commonjs'; + } else if (StringPrototypeEndsWith(filename, '.mjs')) { + format = 'module'; + } else if (StringPrototypeEndsWith(filename, '.js')) { + pkg = packageJsonReader.readPackageScope(filename) || { __proto__: null }; + const typeFromPjson = pkg.data?.type; + if (typeFromPjson === 'module' || typeFromPjson === 'commonjs' || !typeFromPjson) { + format = typeFromPjson; + } } - - module._compile(content, filename, format); + const { source, format: loadedFormat } = loadSource(module, filename, format); + // Function require shouldn't be used in ES modules when require(esm) is disabled. + if (loadedFormat === 'module' && !getOptionValue('--experimental-require-module')) { + const err = getRequireESMError(module, pkg, source, filename); + throw err; + } + module._compile(source, filename, loadedFormat); }; /** From a26abb6ac5bd6b67d2dd8acb278e68dd67987004 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 18 Feb 2025 22:31:55 +0100 Subject: [PATCH 2/9] src: do not format single string argument for THROW_ERR_* If the macros are used as ERR_*(isolate, message) or THROW_ERR_*(isolate, message) with a single string argument, do run formatter on the message, and allow the caller to pass in a message directly with characters that would otherwise need escaping if used as format string unconditionally. PR-URL: https://github.com/nodejs/node/pull/57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel Reviewed-By: Chengzhong Wu --- src/node_errors.h | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/node_errors.h b/src/node_errors.h index d5e2f86f516bbb..23fd33d255e715 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -105,11 +105,21 @@ void OOMErrorHandler(const char* location, const v8::OOMDetails& details); V(ERR_WORKER_INIT_FAILED, Error) \ V(ERR_PROTO_ACCESS, Error) +// If the macros are used as ERR_*(isolate, message) or +// THROW_ERR_*(isolate, message) with a single string argument, do run +// formatter on the message, and allow the caller to pass in a message +// directly with characters that would otherwise need escaping if used +// as format string unconditionally. #define V(code, type) \ template \ inline v8::Local code( \ v8::Isolate* isolate, const char* format, Args&&... args) { \ - std::string message = SPrintF(format, std::forward(args)...); \ + std::string message; \ + if (sizeof...(Args) == 0) { \ + message = format; \ + } else { \ + message = SPrintF(format, std::forward(args)...); \ + } \ v8::Local js_code = OneByteString(isolate, #code); \ v8::Local js_msg = \ v8::String::NewFromUtf8(isolate, \ From 914cc718b6184f5269a181c80191aa6d9d6875c6 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Mon, 17 Feb 2025 19:44:50 +0100 Subject: [PATCH 3/9] module: improve error message from asynchronicity in require(esm) - Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: https://github.com/nodejs/node/pull/57126 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell Reviewed-By: Antoine du Hamel Reviewed-By: Chengzhong Wu --- lib/internal/errors.js | 15 ++++- lib/internal/modules/esm/loader.js | 25 ++++++-- lib/internal/modules/esm/module_job.js | 9 ++- src/module_wrap.cc | 4 +- src/node_errors.h | 26 ++++++-- test/common/index.js | 12 ++++ .../test-require-module-tla-execution.js | 26 ++++++++ .../test-require-module-tla-nested.js | 15 +++++ ...test-require-module-tla-print-execution.js | 29 +++++++++ .../test-require-module-tla-rejected.js | 15 +++++ .../test-require-module-tla-resolved.js | 15 +++++ .../test-require-module-tla-unresolved.js | 15 +++++ test/es-module/test-require-module-tla.js | 63 ------------------- 13 files changed, 190 insertions(+), 79 deletions(-) create mode 100644 test/es-module/test-require-module-tla-execution.js create mode 100644 test/es-module/test-require-module-tla-nested.js create mode 100644 test/es-module/test-require-module-tla-print-execution.js create mode 100644 test/es-module/test-require-module-tla-rejected.js create mode 100644 test/es-module/test-require-module-tla-resolved.js create mode 100644 test/es-module/test-require-module-tla-unresolved.js delete mode 100644 test/es-module/test-require-module-tla.js diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 262f99eb3cefba..9d6c5ca9bb8fdd 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1692,9 +1692,18 @@ 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_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.', Error); +E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parentFilename) { + let message = '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.'; + if (parentFilename) { + message += `\n From ${parentFilename} `; + } + if (filename) { + message += `\n Requiring ${filename} `; + } + return message; +}, Error); E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error); E('ERR_REQUIRE_ESM', function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d9b2c75dd978d0..f026271c4759ba 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -295,20 +295,37 @@ class ModuleLoader { // TODO(joyeecheung): ensure that imported synchronous graphs are evaluated // synchronously so that any previously imported synchronous graph is already // evaluated at this point. + // TODO(joyeecheung): add something similar to CJS loader's requireStack to help + // debugging the the problematic links in the graph for import. if (job !== undefined) { mod[kRequiredModuleSymbol] = job.module; + const parentFilename = urlToFilename(parent?.filename); + // TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous. + if (!job.module) { + let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `; + message += 'This may be caused by a race condition if the module is simultaneously dynamically '; + message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.'; + if (parentFilename) { + message += ` (from ${parentFilename})`; + } + assert(job.module, message); + } if (job.module.async) { - throw new ERR_REQUIRE_ASYNC_MODULE(); + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } + // job.module may be undefined if it's asynchronously loaded. Which means + // there is likely a cycle. 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})`; } + message += 'A cycle involving require(esm) is disallowed to maintain '; + message += 'invariants madated by the ECMAScript specification'; + message += 'Try making at least part of the dependency in the graph lazily loaded.'; throw new ERR_REQUIRE_CYCLE_MODULE(message); } - return { wrap: job.module, namespace: job.module.getNamespaceSync() }; + return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; } // 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 @@ -320,7 +337,7 @@ class ModuleLoader { job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk); this.loadCache.set(url, kImplicitAssertType, job); mod[kRequiredModuleSymbol] = job.module; - return { wrap: job.module, namespace: job.runSync().namespace }; + return { wrap: job.module, namespace: job.runSync(parent).namespace }; } /** diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 0c8f031080a314..a9dd372fe2f498 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -31,6 +31,7 @@ const assert = require('internal/assert'); const resolvedPromise = PromiseResolve(); const { setHasStartedUserESMExecution, + urlToFilename, } = require('internal/modules/helpers'); const { getOptionValue } = require('internal/options'); const noop = FunctionPrototype; @@ -371,7 +372,7 @@ class ModuleJobSync extends ModuleJobBase { `Status = ${status}`); } - runSync() { + runSync(parent) { // TODO(joyeecheung): add the error decoration logic from the async instantiate. this.module.async = this.module.instantiateSync(); // If --experimental-print-required-tla is true, proceeds to evaluation even @@ -380,11 +381,13 @@ class ModuleJobSync extends ModuleJobBase { // TODO(joyeecheung): track the asynchroniticy using v8::Module::HasTopLevelAwait() // and we'll be able to throw right after compilation of the modules, using acron // to find and print the TLA. + const parentFilename = urlToFilename(parent?.filename); + const filename = urlToFilename(this.url); if (this.module.async && !getOptionValue('--experimental-print-required-tla')) { - throw new ERR_REQUIRE_ASYNC_MODULE(); + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } setHasStartedUserESMExecution(); - const namespace = this.module.evaluateSync(); + const namespace = this.module.evaluateSync(filename, parentFilename); return { __proto__: null, module: this.module, namespace }; } } diff --git a/src/module_wrap.cc b/src/module_wrap.cc index a3362e0bafcb11..21aaea106d8d28 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -648,7 +648,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { FPrintF(stderr, "%s\n", reason); } } - THROW_ERR_REQUIRE_ASYNC_MODULE(env); + THROW_ERR_REQUIRE_ASYNC_MODULE(env, args[0], args[1]); return; } @@ -678,7 +678,7 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo& args) { } if (module->IsGraphAsync()) { - return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env()); + return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env(), args[0], args[1]); } Local result = module->GetModuleNamespace(); args.GetReturnValue().Set(result); diff --git a/src/node_errors.h b/src/node_errors.h index 23fd33d255e715..7cacf416b6e624 100644 --- a/src/node_errors.h +++ b/src/node_errors.h @@ -208,10 +208,6 @@ 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") \ @@ -241,6 +237,28 @@ inline void THROW_ERR_SCRIPT_EXECUTION_TIMEOUT(Environment* env, THROW_ERR_SCRIPT_EXECUTION_TIMEOUT(env, message.str().c_str()); } +inline void THROW_ERR_REQUIRE_ASYNC_MODULE( + Environment* env, + v8::Local filename, + v8::Local parent_filename) { + static constexpr const char* prefix = + "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."; + std::string message = prefix; + if (!parent_filename.IsEmpty() && parent_filename->IsString()) { + Utf8Value utf8(env->isolate(), parent_filename); + message += "\n From "; + message += utf8.out(); + } + if (!filename.IsEmpty() && filename->IsString()) { + Utf8Value utf8(env->isolate(), filename); + message += "\n Requiring "; + message += +utf8.out(); + } + THROW_ERR_REQUIRE_ASYNC_MODULE(env, message.c_str()); +} + inline v8::Local ERR_BUFFER_TOO_LARGE(v8::Isolate* isolate) { char message[128]; snprintf(message, sizeof(message), diff --git a/test/common/index.js b/test/common/index.js index 9733f8746baa0e..e860da53a6cd4b 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -937,6 +937,17 @@ function expectRequiredModule(mod, expectation, checkESModule = true) { assert.deepStrictEqual(clone, { ...expectation }); } +function expectRequiredTLAError(err) { + const message = /require\(\) cannot be used on an ESM graph with top-level await/; + if (typeof err === 'string') { + assert.match(err, /ERR_REQUIRE_ASYNC_MODULE/); + assert.match(err, message); + } else { + assert.strictEqual(err.code, 'ERR_REQUIRE_ASYNC_MODULE'); + assert.match(err.message, message); + } +} + const common = { allowGlobals, buildType, @@ -946,6 +957,7 @@ const common = { defaultAutoSelectFamilyAttemptTimeout, expectsError, expectRequiredModule, + expectRequiredTLAError, expectWarning, getArrayBufferViews, getBufferSources, diff --git a/test/es-module/test-require-module-tla-execution.js b/test/es-module/test-require-module-tla-execution.js new file mode 100644 index 00000000000000..bdd222d1f8fa0b --- /dev/null +++ b/test/es-module/test-require-module-tla-execution.js @@ -0,0 +1,26 @@ +'use strict'; + +// Tests that require(esm) with top-level-await throws before execution starts +// if --experimental-print-required-tla is not enabled. + +const common = require('../common'); +const assert = require('assert'); +const { spawnSyncAndExit } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +{ + spawnSyncAndExit(process.execPath, [ + fixtures.path('es-modules/tla/require-execution.js'), + ], { + signal: null, + status: 1, + stderr(output) { + assert.doesNotMatch(output, /I am executed/); + common.expectRequiredTLAError(output); + assert.match(output, /From .*require-execution\.js/); + assert.match(output, /Requiring .*execution\.mjs/); + return true; + }, + stdout: '' + }); +} diff --git a/test/es-module/test-require-module-tla-nested.js b/test/es-module/test-require-module-tla-nested.js new file mode 100644 index 00000000000000..583cd7cd0c95db --- /dev/null +++ b/test/es-module/test-require-module-tla-nested.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for top-level-await in inner graphs. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/parent.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-nested\.js/); + assert.match(err.message, /Requiring .*parent\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla-print-execution.js b/test/es-module/test-require-module-tla-print-execution.js new file mode 100644 index 00000000000000..40992ae32e0905 --- /dev/null +++ b/test/es-module/test-require-module-tla-print-execution.js @@ -0,0 +1,29 @@ +'use strict'; + +// Tests that require(esm) with top-level-await throws after execution +// if --experimental-print-required-tla is enabled. + +const common = require('../common'); +const assert = require('assert'); +const { spawnSyncAndExit } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +{ + spawnSyncAndExit(process.execPath, [ + '--experimental-print-required-tla', + fixtures.path('es-modules/tla/require-execution.js'), + ], { + signal: null, + status: 1, + stderr(output) { + assert.match(output, /I am executed/); + common.expectRequiredTLAError(output); + assert.match(output, /Error: unexpected top-level await at.*execution\.mjs:3/); + assert.match(output, /await Promise\.resolve\('hi'\)/); + assert.match(output, /From .*require-execution\.js/); + assert.match(output, /Requiring .*execution\.mjs/); + return true; + }, + stdout: '' + }); +} diff --git a/test/es-module/test-require-module-tla-rejected.js b/test/es-module/test-require-module-tla-rejected.js new file mode 100644 index 00000000000000..0c1f8de2c307f6 --- /dev/null +++ b/test/es-module/test-require-module-tla-rejected.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for rejected top-level await. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/rejected.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-rejected\.js/); + assert.match(err.message, /Requiring .*rejected\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla-resolved.js b/test/es-module/test-require-module-tla-resolved.js new file mode 100644 index 00000000000000..f35bb68b7dc180 --- /dev/null +++ b/test/es-module/test-require-module-tla-resolved.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for resolved top-level-await. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/resolved.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-resolved\.js/); + assert.match(err.message, /Requiring .*resolved\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla-unresolved.js b/test/es-module/test-require-module-tla-unresolved.js new file mode 100644 index 00000000000000..35cf12c446129b --- /dev/null +++ b/test/es-module/test-require-module-tla-unresolved.js @@ -0,0 +1,15 @@ +'use strict'; + +// Tests that require(esm) throws for unresolved top-level-await. + +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/tla/unresolved.mjs'); +}, (err) => { + common.expectRequiredTLAError(err); + assert.match(err.message, /From .*test-require-module-tla-unresolved\.js/); + assert.match(err.message, /Requiring .*unresolved\.mjs/); + return true; +}); diff --git a/test/es-module/test-require-module-tla.js b/test/es-module/test-require-module-tla.js deleted file mode 100644 index 9b38b1cab3fcb5..00000000000000 --- a/test/es-module/test-require-module-tla.js +++ /dev/null @@ -1,63 +0,0 @@ -// 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: '' - }); -} From d52043a41b7d3fe975eb7fd19df96d784617a726 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 4 Mar 2025 08:34:34 +0100 Subject: [PATCH 4/9] module: handle cached linked async jobs in require(esm) This handles two cases caused by using Promise.all() with multiple dynamic import() that can make an asynchronously linked module job that has finished/failed linking but has not yet started actual evaluation appear in the load cache when another require request is in turn to handle it. - When the cached async job has finished linking but has not started its evaluation because the async run() task would be later in line, start the evaluation from require() with runSync(). - When the cached async job have already encountered a linking error that gets wrapped into a rejection, but is still later in line to throw on it, just unwrap and throw the linking error from require(). PR-URL: https://github.com/nodejs/node/pull/57187 Fixes: https://github.com/nodejs/node/issues/57172 Refs: https://github.com/shufo/prettier-plugin-blade/issues/311 Refs: https://github.com/fisker/prettier-plugin-blade-311 Refs: https://github.com/mochajs/mocha/issues/5290 Refs: https://github.com/JoshuaKGoldberg/repros/tree/mocha-missing-module-cyclic Reviewed-By: Guy Bedford Reviewed-By: Matteo Collina --- lib/internal/modules/esm/loader.js | 85 ++++++++++++++++++++------ lib/internal/modules/esm/module_job.js | 10 +-- src/module_wrap.cc | 71 ++++++++++++++------- 3 files changed, 118 insertions(+), 48 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index f026271c4759ba..90c71214c82733 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -36,12 +36,22 @@ const { getDefaultConditions, } = require('internal/modules/esm/utils'); const { kImplicitAssertType } = require('internal/modules/esm/assert'); -const { ModuleWrap, kEvaluating, kEvaluated } = internalBinding('module_wrap'); +const { + ModuleWrap, + kEvaluated, + kEvaluating, + kInstantiated, + throwIfPromiseRejected, +} = internalBinding('module_wrap'); const { urlToFilename, } = require('internal/modules/helpers'); let defaultResolve, defaultLoad, defaultLoadSync, importMetaInitializer; +let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { + debug = fn; +}); + /** * @typedef {import('./hooks.js').HooksProxy} HooksProxy * @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase @@ -75,6 +85,23 @@ function getTranslators() { return translators; } +/** + * Generate message about potential race condition caused by requiring a cached module that has started + * async linking. + * @param {string} filename Filename of the module being required. + * @param {string|undefined} parentFilename Filename of the module calling require(). + * @returns {string} Error message. + */ +function getRaceMessage(filename, parentFilename) { + let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `; + raceMessage += 'This may be caused by a race condition if the module is simultaneously dynamically '; + raceMessage += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.'; + if (parentFilename) { + raceMessage += ` (from ${parentFilename})`; + } + return raceMessage; +} + /** * @type {HooksProxy} * Multiple loader instances exist for various, specific reasons (see code comments at site). @@ -297,35 +324,53 @@ class ModuleLoader { // evaluated at this point. // TODO(joyeecheung): add something similar to CJS loader's requireStack to help // debugging the the problematic links in the graph for import. + debug('importSyncForRequire', parent?.filename, '->', filename, job); if (job !== undefined) { mod[kRequiredModuleSymbol] = job.module; const parentFilename = urlToFilename(parent?.filename); // TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous. if (!job.module) { - let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `; - message += 'This may be caused by a race condition if the module is simultaneously dynamically '; - message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.'; - if (parentFilename) { - message += ` (from ${parentFilename})`; - } - assert(job.module, message); + assert.fail(getRaceMessage(filename, parentFilename)); } if (job.module.async) { throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } - // job.module may be undefined if it's asynchronously loaded. Which means - // there is likely a cycle. - if (job.module.getStatus() !== kEvaluated) { - let message = `Cannot require() ES Module ${filename} in a cycle.`; - if (parentFilename) { - message += ` (from ${parentFilename})`; - } - message += 'A cycle involving require(esm) is disallowed to maintain '; - message += 'invariants madated by the ECMAScript specification'; - message += 'Try making at least part of the dependency in the graph lazily loaded.'; - throw new ERR_REQUIRE_CYCLE_MODULE(message); + const status = job.module.getStatus(); + debug('Module status', filename, status); + if (status === kEvaluated) { + return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; + } else if (status === kInstantiated) { + // When it's an async job cached by another import request, + // which has finished linking but has not started its + // evaluation because the async run() task would be later + // in line. Then start the evaluation now with runSync(), which + // is guaranteed to finish by the time the other run() get to it, + // and the other task would just get the cached evaluation results, + // similar to what would happen when both are async. + mod[kRequiredModuleSymbol] = job.module; + const { namespace } = job.runSync(parent); + return { wrap: job.module, namespace: namespace || job.module.getNamespace() }; } - return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; + // When the cached async job have already encountered a linking + // error that gets wrapped into a rejection, but is still later + // in line to throw on it, just unwrap and throw the linking error + // from require(). + if (job.instantiated) { + throwIfPromiseRejected(job.instantiated); + } + if (status !== kEvaluating) { + assert.fail(`Unexpected module status ${status}. ` + + getRaceMessage(filename, parentFilename)); + } + let message = `Cannot require() ES Module ${filename} in a cycle.`; + if (parentFilename) { + message += ` (from ${parentFilename})`; + } + message += 'A cycle involving require(esm) is disallowed to maintain '; + message += 'invariants madated by the ECMAScript specification'; + message += 'Try making at least part of the dependency in the graph lazily loaded.'; + throw new ERR_REQUIRE_CYCLE_MODULE(message); + } // 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 diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index a9dd372fe2f498..31da9693c349d9 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -240,7 +240,7 @@ class ModuleJob extends ModuleJobBase { } } - runSync() { + runSync(parent) { assert(this.module instanceof ModuleWrap); if (this.instantiated !== undefined) { return { __proto__: null, module: this.module }; @@ -248,11 +248,11 @@ class ModuleJob extends ModuleJobBase { this.module.instantiate(); this.instantiated = PromiseResolve(); - const timeout = -1; - const breakOnSigint = false; setHasStartedUserESMExecution(); - this.module.evaluate(timeout, breakOnSigint); - return { __proto__: null, module: this.module }; + const filename = urlToFilename(this.url); + const parentFilename = urlToFilename(parent?.filename); + const namespace = this.module.evaluateSync(filename, parentFilename); + return { __proto__: null, module: this.module, namespace }; } async run() { diff --git a/src/module_wrap.cc b/src/module_wrap.cc index 21aaea106d8d28..c8594dc9b196fc 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -24,6 +24,7 @@ using v8::Array; using v8::ArrayBufferView; using v8::Context; using v8::EscapableHandleScope; +using v8::Exception; using v8::FixedArray; using v8::Function; using v8::FunctionCallbackInfo; @@ -32,15 +33,22 @@ using v8::HandleScope; using v8::Int32; using v8::Integer; using v8::Isolate; +using v8::JustVoid; using v8::Local; +using v8::Maybe; using v8::MaybeLocal; +using v8::Message; using v8::MicrotaskQueue; using v8::Module; using v8::ModuleRequest; +using v8::Name; +using v8::Nothing; +using v8::Null; using v8::Object; using v8::ObjectTemplate; using v8::PrimitiveArray; using v8::Promise; +using v8::PromiseRejectEvent; using v8::ScriptCompiler; using v8::ScriptOrigin; using v8::String; @@ -584,6 +592,43 @@ void ModuleWrap::InstantiateSync(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(module->IsGraphAsync()); } +Maybe ThrowIfPromiseRejected(Realm* realm, Local promise) { + Isolate* isolate = realm->isolate(); + Local context = realm->context(); + if (promise->State() != Promise::PromiseState::kRejected) { + return JustVoid(); + } + // The rejected promise is created by V8, so we don't get a chance to mark + // it as resolved before the rejection happens from evaluation. But we can + // tell the promise rejection callback to treat it as a promise rejected + // before handler was added which would remove it from the unhandled + // rejection handling, since we are converting it into an error and throw + // from here directly. + Local type = + Integer::New(isolate, + static_cast( + PromiseRejectEvent::kPromiseHandlerAddedAfterReject)); + Local args[] = {type, promise, Undefined(isolate)}; + if (realm->promise_reject_callback() + ->Call(context, Undefined(isolate), arraysize(args), args) + .IsEmpty()) { + return Nothing(); + } + Local exception = promise->Result(); + Local message = Exception::CreateMessage(isolate, exception); + AppendExceptionLine( + realm->env(), exception, message, ErrorHandlingMode::MODULE_ERROR); + isolate->ThrowException(exception); + return Nothing(); +} + +void ThrowIfPromiseRejected(const FunctionCallbackInfo& args) { + if (!args[0]->IsPromise()) { + return; + } + ThrowIfPromiseRejected(Realm::GetCurrent(args), args[0].As()); +} + void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { Realm* realm = Realm::GetCurrent(args); Isolate* isolate = args.GetIsolate(); @@ -608,29 +653,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo& args) { CHECK(result->IsPromise()); Local promise = result.As(); - if (promise->State() == Promise::PromiseState::kRejected) { - // The rejected promise is created by V8, so we don't get a chance to mark - // it as resolved before the rejection happens from evaluation. But we can - // tell the promise rejection callback to treat it as a promise rejected - // before handler was added which would remove it from the unhandled - // rejection handling, since we are converting it into an error and throw - // from here directly. - Local type = v8::Integer::New( - isolate, - static_cast( - v8::PromiseRejectEvent::kPromiseHandlerAddedAfterReject)); - Local args[] = {type, promise, Undefined(isolate)}; - if (env->promise_reject_callback() - ->Call(context, Undefined(isolate), arraysize(args), args) - .IsEmpty()) { - return; - } - Local exception = promise->Result(); - Local message = - v8::Exception::CreateMessage(isolate, exception); - AppendExceptionLine( - env, exception, message, ErrorHandlingMode::MODULE_ERROR); - isolate->ThrowException(exception); + if (ThrowIfPromiseRejected(realm, promise).IsNothing()) { return; } @@ -1083,6 +1106,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data, target, "createRequiredModuleFacade", CreateRequiredModuleFacade); + SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected); } void ModuleWrap::CreatePerContextProperties(Local target, @@ -1127,6 +1151,7 @@ void ModuleWrap::RegisterExternalReferences( registry->Register(SetImportModuleDynamicallyCallback); registry->Register(SetInitializeImportMetaObjectCallback); + registry->Register(ThrowIfPromiseRejected); } } // namespace loader } // namespace node From feb6167ded4e8d55369ff66d4a41a03800631726 Mon Sep 17 00:00:00 2001 From: haykam821 <24855774+haykam821@users.noreply.github.com> Date: Wed, 9 Apr 2025 07:47:58 -0400 Subject: [PATCH 5/9] module: fix incorrect formatting in require(esm) cycle error message Fixes: https://github.com/nodejs/node/issues/57451 PR-URL: https://github.com/nodejs/node/pull/57453 Reviewed-By: Marco Ippolito Reviewed-By: Ruben Bridgewater Reviewed-By: Jake Yuesong Li Reviewed-By: Luigi Pinca Reviewed-By: James M Snell --- lib/internal/modules/esm/loader.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 90c71214c82733..db7e83fe83d359 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -366,8 +366,8 @@ class ModuleLoader { if (parentFilename) { message += ` (from ${parentFilename})`; } - message += 'A cycle involving require(esm) is disallowed to maintain '; - message += 'invariants madated by the ECMAScript specification'; + message += ' A cycle involving require(esm) is not allowed to maintain '; + message += 'invariants mandated by the ECMAScript specification. '; message += 'Try making at least part of the dependency in the graph lazily loaded.'; throw new ERR_REQUIRE_CYCLE_MODULE(message); From 08ff294e801aec940b79fe2c10517ff7f4a337d0 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Thu, 8 May 2025 22:10:32 +0200 Subject: [PATCH 6/9] module: handle instantiated async module jobs in require(esm) When require(esm) encounters a cached module job that is instantiated but not yet evaluated, run the evaluation. This catches an edge case previously missed in https://github.com/nodejs/node/pull/57187. PR-URL: https://github.com/nodejs/node/pull/58067 Fixes: https://github.com/nodejs/node/issues/58061 Reviewed-By: Jacob Smith --- lib/internal/modules/esm/loader.js | 2 +- lib/internal/modules/esm/module_job.js | 44 ++++++++++++++----- src/debug_utils.h | 1 + src/module_wrap.cc | 12 +++++ src/module_wrap.h | 1 + .../test-require-module-instantiated.mjs | 4 ++ .../require-module-instantiated/a.mjs | 2 + .../require-module-instantiated/b.cjs | 1 + .../require-module-instantiated/c.mjs | 3 ++ 9 files changed, 58 insertions(+), 12 deletions(-) create mode 100644 test/es-module/test-require-module-instantiated.mjs create mode 100644 test/fixtures/es-modules/require-module-instantiated/a.mjs create mode 100644 test/fixtures/es-modules/require-module-instantiated/b.cjs create mode 100644 test/fixtures/es-modules/require-module-instantiated/c.mjs diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index db7e83fe83d359..6357d174fcd989 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -336,7 +336,7 @@ class ModuleLoader { throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } const status = job.module.getStatus(); - debug('Module status', filename, status); + debug('Module status', job, status); if (status === kEvaluated) { return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) }; } else if (status === kInstantiated) { diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 31da9693c349d9..e92ffd637261ef 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -21,8 +21,13 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); -const { ModuleWrap, kInstantiated } = internalBinding('module_wrap'); - +const { + ModuleWrap, + kErrored, + kEvaluated, + kInstantiated, + kUninstantiated, +} = internalBinding('module_wrap'); const { decorateErrorStack, kEmptyObject } = require('internal/util'); const { getSourceMapsEnabled, @@ -242,17 +247,34 @@ class ModuleJob extends ModuleJobBase { runSync(parent) { assert(this.module instanceof ModuleWrap); - if (this.instantiated !== undefined) { - return { __proto__: null, module: this.module }; + let status = this.module.getStatus(); + + debug('ModuleJob.runSync', this.module); + // FIXME(joyeecheung): this cannot fully handle < kInstantiated. Make the linking + // fully synchronous instead. + if (status === kUninstantiated) { + this.module.async = this.module.instantiateSync(); + status = this.module.getStatus(); } + if (status === kInstantiated || status === kErrored) { + const filename = urlToFilename(this.url); + const parentFilename = urlToFilename(parent?.filename); + this.module.async ??= this.module.isGraphAsync(); - this.module.instantiate(); - this.instantiated = PromiseResolve(); - setHasStartedUserESMExecution(); - const filename = urlToFilename(this.url); - const parentFilename = urlToFilename(parent?.filename); - const namespace = this.module.evaluateSync(filename, parentFilename); - return { __proto__: null, module: this.module, namespace }; + if (this.module.async && !getOptionValue('--experimental-print-required-tla')) { + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); + } + if (status === kInstantiated) { + setHasStartedUserESMExecution(); + const namespace = this.module.evaluateSync(filename, parentFilename); + return { __proto__: null, module: this.module, namespace }; + } + throw this.module.getError(); + + } else if (status === kEvaluated) { + return { __proto__: null, module: this.module, namespace: this.module.getNamespaceSync() }; + } + assert.fail(`Unexpected module status ${status}.`); } async run() { diff --git a/src/debug_utils.h b/src/debug_utils.h index d00e2d1ca9ac4b..95431ac83960f9 100644 --- a/src/debug_utils.h +++ b/src/debug_utils.h @@ -51,6 +51,7 @@ void NODE_EXTERN_PRIVATE FWrite(FILE* file, const std::string& str); V(NGTCP2_DEBUG) \ V(SEA) \ V(WASI) \ + V(MODULE) \ V(MKSNAPSHOT) \ V(SNAPSHOT_SERDES) \ V(PERMISSION_MODEL) diff --git a/src/module_wrap.cc b/src/module_wrap.cc index c8594dc9b196fc..d4a9f8f00acfda 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -743,6 +743,16 @@ void ModuleWrap::GetStatus(const FunctionCallbackInfo& args) { args.GetReturnValue().Set(module->GetStatus()); } +void ModuleWrap::IsGraphAsync(const FunctionCallbackInfo& args) { + Isolate* isolate = args.GetIsolate(); + ModuleWrap* obj; + ASSIGN_OR_RETURN_UNWRAP(&obj, args.This()); + + Local module = obj->module_.Get(isolate); + + args.GetReturnValue().Set(module->IsGraphAsync()); +} + void ModuleWrap::GetError(const FunctionCallbackInfo& args) { Isolate* isolate = args.GetIsolate(); ModuleWrap* obj; @@ -1090,6 +1100,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data, isolate, tpl, "createCachedData", CreateCachedData); SetProtoMethodNoSideEffect(isolate, tpl, "getNamespace", GetNamespace); SetProtoMethodNoSideEffect(isolate, tpl, "getStatus", GetStatus); + SetProtoMethodNoSideEffect(isolate, tpl, "isGraphAsync", IsGraphAsync); SetProtoMethodNoSideEffect(isolate, tpl, "getError", GetError); SetConstructorFunction(isolate, target, "ModuleWrap", tpl); isolate_data->set_module_wrap_constructor_template(tpl); @@ -1146,6 +1157,7 @@ void ModuleWrap::RegisterExternalReferences( registry->Register(GetNamespace); registry->Register(GetStatus); registry->Register(GetError); + registry->Register(IsGraphAsync); registry->Register(CreateRequiredModuleFacade); diff --git a/src/module_wrap.h b/src/module_wrap.h index 51b127209af695..860495d8db6900 100644 --- a/src/module_wrap.h +++ b/src/module_wrap.h @@ -110,6 +110,7 @@ class ModuleWrap : public BaseObject { static void Instantiate(const v8::FunctionCallbackInfo& args); static void Evaluate(const v8::FunctionCallbackInfo& args); static void GetNamespace(const v8::FunctionCallbackInfo& args); + static void IsGraphAsync(const v8::FunctionCallbackInfo& args); static void GetStatus(const v8::FunctionCallbackInfo& args); static void GetError(const v8::FunctionCallbackInfo& args); diff --git a/test/es-module/test-require-module-instantiated.mjs b/test/es-module/test-require-module-instantiated.mjs new file mode 100644 index 00000000000000..9dd50f31d36fd4 --- /dev/null +++ b/test/es-module/test-require-module-instantiated.mjs @@ -0,0 +1,4 @@ +import '../common/index.mjs'; +import assert from 'node:assert'; +import { b, c } from '../fixtures/es-modules/require-module-instantiated/a.mjs'; +assert.strictEqual(b, c); diff --git a/test/fixtures/es-modules/require-module-instantiated/a.mjs b/test/fixtures/es-modules/require-module-instantiated/a.mjs new file mode 100644 index 00000000000000..2918d41462152f --- /dev/null +++ b/test/fixtures/es-modules/require-module-instantiated/a.mjs @@ -0,0 +1,2 @@ +export { default as b } from './b.cjs'; +export { default as c } from './c.mjs'; diff --git a/test/fixtures/es-modules/require-module-instantiated/b.cjs b/test/fixtures/es-modules/require-module-instantiated/b.cjs new file mode 100644 index 00000000000000..1e23a5d46d2ad2 --- /dev/null +++ b/test/fixtures/es-modules/require-module-instantiated/b.cjs @@ -0,0 +1 @@ +module.exports = require('./c.mjs'); diff --git a/test/fixtures/es-modules/require-module-instantiated/c.mjs b/test/fixtures/es-modules/require-module-instantiated/c.mjs new file mode 100644 index 00000000000000..a5b4faccf9a4fd --- /dev/null +++ b/test/fixtures/es-modules/require-module-instantiated/c.mjs @@ -0,0 +1,3 @@ +const foo = 1; +export default foo; +export { foo as 'module.exports' }; From 10c4efdd74875c076fb65baa5c3b625bd3b4e950 Mon Sep 17 00:00:00 2001 From: Carlos Espa <43477095+Ceres6@users.noreply.github.com> Date: Fri, 16 May 2025 08:39:28 +0200 Subject: [PATCH 7/9] module: clarify cjs global-like error on ModuleJobSync PR-URL: https://github.com/nodejs/node/pull/56491 Reviewed-By: James M Snell Reviewed-By: Joyee Cheung Reviewed-By: Jacob Smith --- lib/internal/modules/esm/module_job.js | 62 ++++++++++++------- .../test-require-module-error-catching.js | 2 +- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index e92ffd637261ef..13d383e436edfb 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -58,6 +58,37 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) => (globalLike) => errorMessage === `${globalLike} is not defined`, ); + +/** + * + * @param {Error} e + * @param {string} url + * @returns {void} + */ +const explainCommonJSGlobalLikeNotDefinedError = (e, url) => { + if (e?.name === 'ReferenceError' && + isCommonJSGlobalLikeNotDefinedError(e.message)) { + e.message += ' in ES module scope'; + + if (StringPrototypeStartsWith(e.message, 'require ')) { + e.message += ', you can use import instead'; + } + + const packageConfig = + StringPrototypeStartsWith(url, 'file://') && + RegExpPrototypeExec(/\.js(\?[^#]*)?(#.*)?$/, url) !== null && + require('internal/modules/esm/resolve') + .getPackageScopeConfig(url); + if (packageConfig.type === 'module') { + e.message += + '\nThis file is being treated as an ES module because it has a ' + + `'.js' file extension and '${packageConfig.pjsonPath}' contains ` + + '"type": "module". To treat it as a CommonJS script, rename it ' + + 'to use the \'.cjs\' file extension.'; + } + } +}; + class ModuleJobBase { constructor(url, importAttributes, isMain, inspectBrk) { this.importAttributes = importAttributes; @@ -285,27 +316,7 @@ class ModuleJob extends ModuleJobBase { try { await this.module.evaluate(timeout, breakOnSigint); } catch (e) { - if (e?.name === 'ReferenceError' && - isCommonJSGlobalLikeNotDefinedError(e.message)) { - e.message += ' in ES module scope'; - - if (StringPrototypeStartsWith(e.message, 'require ')) { - e.message += ', you can use import instead'; - } - - const packageConfig = - StringPrototypeStartsWith(this.module.url, 'file://') && - RegExpPrototypeExec(/\.js(\?[^#]*)?(#.*)?$/, this.module.url) !== null && - require('internal/modules/esm/resolve') - .getPackageScopeConfig(this.module.url); - if (packageConfig.type === 'module') { - e.message += - '\nThis file is being treated as an ES module because it has a ' + - `'.js' file extension and '${packageConfig.pjsonPath}' contains ` + - '"type": "module". To treat it as a CommonJS script, rename it ' + - 'to use the \'.cjs\' file extension.'; - } - } + explainCommonJSGlobalLikeNotDefinedError(e, this.module.url); throw e; } return { __proto__: null, module: this.module }; @@ -409,8 +420,13 @@ class ModuleJobSync extends ModuleJobBase { throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } setHasStartedUserESMExecution(); - const namespace = this.module.evaluateSync(filename, parentFilename); - return { __proto__: null, module: this.module, namespace }; + try { + const namespace = this.module.evaluateSync(filename, parentFilename); + return { __proto__: null, module: this.module, namespace }; + } catch (e) { + explainCommonJSGlobalLikeNotDefinedError(e, this.module.url); + throw e; + } } } diff --git a/test/es-module/test-require-module-error-catching.js b/test/es-module/test-require-module-error-catching.js index c314513d9bbf04..5400564b3182c6 100644 --- a/test/es-module/test-require-module-error-catching.js +++ b/test/es-module/test-require-module-error-catching.js @@ -17,5 +17,5 @@ assert.throws(() => { require('../fixtures/es-modules/reference-error-esm.js'); }, { name: 'ReferenceError', - message: 'exports is not defined' + message: 'exports is not defined in ES module scope' }); From b29b87400846105e674051a2c9f4c850510eff19 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 11 Jun 2025 11:33:16 +0200 Subject: [PATCH 8/9] module: allow cycles in require() in the CJS handling in ESM loader When --import is used, the ESM loader is used to handle even pure CJS entry points, and it can run into CJS module facades in the evaluating state when the parent CJS module is being evaluated. In this case it should be allowed, since the ESM <-> CJS cycles that are meant to be disallowed (for the time being) should already be detected before evaluation and wouldn't get here, and CJS <-> CJS cycles are fine. PR-URL: https://github.com/nodejs/node/pull/58598 Fixes: https://github.com/nodejs/node/issues/58515 Reviewed-By: Anna Henningsen Reviewed-By: Ethan Arrowood --- lib/internal/modules/esm/module_job.js | 11 ++++++++-- src/module_wrap.cc | 3 +-- .../test-import-preload-require-cycle.js | 20 +++++++++++++++++++ test/fixtures/import-require-cycle/a.js | 1 + test/fixtures/import-require-cycle/b.js | 1 + test/fixtures/import-require-cycle/c.js | 3 +++ .../fixtures/import-require-cycle/preload.mjs | 10 ++++++++++ 7 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 test/es-module/test-import-preload-require-cycle.js create mode 100644 test/fixtures/import-require-cycle/a.js create mode 100644 test/fixtures/import-require-cycle/b.js create mode 100644 test/fixtures/import-require-cycle/c.js create mode 100644 test/fixtures/import-require-cycle/preload.mjs diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 13d383e436edfb..0ece92157fe1f3 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -25,6 +25,7 @@ const { ModuleWrap, kErrored, kEvaluated, + kEvaluating, kInstantiated, kUninstantiated, } = internalBinding('module_wrap'); @@ -301,8 +302,14 @@ class ModuleJob extends ModuleJobBase { return { __proto__: null, module: this.module, namespace }; } throw this.module.getError(); - - } else if (status === kEvaluated) { + } else if (status === kEvaluating || status === kEvaluated) { + // kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles. + // Allow it for now, since we only need to ban ESM <-> CJS cycles which would be + // detected earlier during the linking phase, though the CJS handling in the ESM + // loader won't be able to emit warnings on pending circular exports like what + // the CJS loader does. + // TODO(joyeecheung): remove the re-invented require() in the ESM loader and + // always handle CJS using the CJS loader to eliminate the quirks. return { __proto__: null, module: this.module, namespace: this.module.getNamespaceSync() }; } assert.fail(`Unexpected module status ${status}.`); diff --git a/src/module_wrap.cc b/src/module_wrap.cc index d4a9f8f00acfda..355b387e1b7b7a 100644 --- a/src/module_wrap.cc +++ b/src/module_wrap.cc @@ -693,11 +693,10 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo& args) { return realm->env()->ThrowError( "Cannot get namespace, module has not been instantiated"); case v8::Module::Status::kInstantiated: + case v8::Module::Status::kEvaluating: case v8::Module::Status::kEvaluated: case v8::Module::Status::kErrored: break; - case v8::Module::Status::kEvaluating: - UNREACHABLE(); } if (module->IsGraphAsync()) { diff --git a/test/es-module/test-import-preload-require-cycle.js b/test/es-module/test-import-preload-require-cycle.js new file mode 100644 index 00000000000000..c47b64ba584802 --- /dev/null +++ b/test/es-module/test-import-preload-require-cycle.js @@ -0,0 +1,20 @@ +'use strict'; + +// This tests that --import preload does not break CJS entry points that contains +// require cycles. + +require('../common'); +const fixtures = require('../common/fixtures'); +const { spawnSyncAndAssert } = require('../common/child_process'); + +spawnSyncAndAssert( + process.execPath, + [ + '--import', + fixtures.fileURL('import-require-cycle/preload.mjs'), + fixtures.path('import-require-cycle/c.js'), + ], + { + stdout: /cycle equality true/, + } +); diff --git a/test/fixtures/import-require-cycle/a.js b/test/fixtures/import-require-cycle/a.js new file mode 100644 index 00000000000000..595a5085cf5ff9 --- /dev/null +++ b/test/fixtures/import-require-cycle/a.js @@ -0,0 +1 @@ +module.exports.b = require('./b.js'); diff --git a/test/fixtures/import-require-cycle/b.js b/test/fixtures/import-require-cycle/b.js new file mode 100644 index 00000000000000..869be257319c1c --- /dev/null +++ b/test/fixtures/import-require-cycle/b.js @@ -0,0 +1 @@ +module.exports.a = require('./a.js'); diff --git a/test/fixtures/import-require-cycle/c.js b/test/fixtures/import-require-cycle/c.js new file mode 100644 index 00000000000000..39099ad76074b3 --- /dev/null +++ b/test/fixtures/import-require-cycle/c.js @@ -0,0 +1,3 @@ +const obj = require('./b.js'); + +console.log('cycle equality', obj.a.b === obj); diff --git a/test/fixtures/import-require-cycle/preload.mjs b/test/fixtures/import-require-cycle/preload.mjs new file mode 100644 index 00000000000000..83690b86feafb3 --- /dev/null +++ b/test/fixtures/import-require-cycle/preload.mjs @@ -0,0 +1,10 @@ +import * as mod from "module"; + +// This API is not available on v20.x. We are just checking that a +// using a --import preload to force the ESM loader to load CJS +// still handles CJS <-> CJS cycles just fine. +// mod.registerHooks({ +// load(url, context, nextLoad) { +// return nextLoad(url, context); +// }, +// }); From bd6691a3bd9e4df2d1cce9cd0e43807bfb91e4f9 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Wed, 9 Jul 2025 18:39:29 +0200 Subject: [PATCH 9/9] module: throw error when re-runing errored module jobs Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: https://github.com/nodejs/node/pull/58957 Fixes: https://github.com/nodejs/node/issues/58945 Reviewed-By: Marco Ippolito Reviewed-By: Luigi Pinca Reviewed-By: Antoine du Hamel --- lib/internal/modules/esm/loader.js | 4 ++++ lib/internal/modules/esm/module_job.js | 10 ++++++++-- .../test-import-module-retry-require-errored.js | 17 +++++++++++++++++ ...est-require-module-retry-import-errored-2.js | 16 ++++++++++++++++ 4 files changed, 45 insertions(+), 2 deletions(-) create mode 100644 test/es-module/test-import-module-retry-require-errored.js create mode 100644 test/es-module/test-require-module-retry-import-errored-2.js diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 6357d174fcd989..e85f4b3acc3be1 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -41,6 +41,7 @@ const { kEvaluated, kEvaluating, kInstantiated, + kErrored, throwIfPromiseRejected, } = internalBinding('module_wrap'); const { @@ -350,6 +351,9 @@ class ModuleLoader { mod[kRequiredModuleSymbol] = job.module; const { namespace } = job.runSync(parent); return { wrap: job.module, namespace: namespace || job.module.getNamespace() }; + } else if (status === kErrored) { + // If the module was previously imported and errored, throw the error. + throw job.module.getError(); } // When the cached async job have already encountered a linking // error that gets wrapped into a rejection, but is still later diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index 0ece92157fe1f3..3b68647e455d20 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -281,7 +281,7 @@ class ModuleJob extends ModuleJobBase { assert(this.module instanceof ModuleWrap); let status = this.module.getStatus(); - debug('ModuleJob.runSync', this.module); + debug('ModuleJob.runSync()', status, this.module); // FIXME(joyeecheung): this cannot fully handle < kInstantiated. Make the linking // fully synchronous instead. if (status === kUninstantiated) { @@ -316,6 +316,7 @@ class ModuleJob extends ModuleJobBase { } async run() { + debug('ModuleJob.run()', this.module); await this.instantiate(); const timeout = -1; const breakOnSigint = false; @@ -392,7 +393,11 @@ class ModuleJobSync extends ModuleJobBase { async run() { // This path is hit by a require'd module that is imported again. const status = this.module.getStatus(); - if (status > kInstantiated) { + debug('ModuleJobSync.run()', status, this.module); + // If the module was previously required and errored, reject from import() again. + if (status === kErrored) { + throw this.module.getError(); + } else if (status > kInstantiated) { if (this.evaluationPromise) { await this.evaluationPromise; } @@ -413,6 +418,7 @@ class ModuleJobSync extends ModuleJobBase { } runSync(parent) { + debug('ModuleJobSync.runSync()', this.module); // TODO(joyeecheung): add the error decoration logic from the async instantiate. this.module.async = this.module.instantiateSync(); // If --experimental-print-required-tla is true, proceeds to evaluation even diff --git a/test/es-module/test-import-module-retry-require-errored.js b/test/es-module/test-import-module-retry-require-errored.js new file mode 100644 index 00000000000000..79afa20aa70cb5 --- /dev/null +++ b/test/es-module/test-import-module-retry-require-errored.js @@ -0,0 +1,17 @@ +// This tests that after failing to import an ESM that rejects, +// retrying with require() still throws. + +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +(async () => { + await assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), { + message: 'test', + }); + assert.throws(() => { + require('../fixtures/es-modules/throw-error.mjs'); + }, { + message: 'test', + }); +})().catch(common.mustNotCall()); diff --git a/test/es-module/test-require-module-retry-import-errored-2.js b/test/es-module/test-require-module-retry-import-errored-2.js new file mode 100644 index 00000000000000..3504d9932c61af --- /dev/null +++ b/test/es-module/test-require-module-retry-import-errored-2.js @@ -0,0 +1,16 @@ +// This tests that after failing to require an ESM that throws, +// retrying with import() still rejects. + +'use strict'; +const common = require('../common'); +const assert = require('assert'); + +assert.throws(() => { + require('../fixtures/es-modules/throw-error.mjs'); +}, { + message: 'test', +}); + +assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), { + message: 'test', +}).catch(common.mustNotCall());