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/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); }; /** diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d9b2c75dd978d0..e85f4b3acc3be1 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -36,12 +36,23 @@ 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, + kErrored, + 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 +86,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). @@ -295,20 +323,58 @@ 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. + 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) { + assert.fail(getRaceMessage(filename, parentFilename)); + } if (job.module.async) { - throw new ERR_REQUIRE_ASYNC_MODULE(); + throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename); } - 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); + const status = job.module.getStatus(); + debug('Module status', job, 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() }; + } 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 + // in line to throw on it, just unwrap and throw the linking error + // from require(). + if (job.instantiated) { + throwIfPromiseRejected(job.instantiated); } - return { wrap: job.module, namespace: job.module.getNamespaceSync() }; + 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 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); + } // 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 +386,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..3b68647e455d20 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -21,8 +21,14 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => { debug = fn; }); -const { ModuleWrap, kInstantiated } = internalBinding('module_wrap'); - +const { + ModuleWrap, + kErrored, + kEvaluated, + kEvaluating, + kInstantiated, + kUninstantiated, +} = internalBinding('module_wrap'); const { decorateErrorStack, kEmptyObject } = require('internal/util'); const { getSourceMapsEnabled, @@ -31,6 +37,7 @@ const assert = require('internal/assert'); const resolvedPromise = PromiseResolve(); const { setHasStartedUserESMExecution, + urlToFilename, } = require('internal/modules/helpers'); const { getOptionValue } = require('internal/options'); const noop = FunctionPrototype; @@ -52,6 +59,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; @@ -239,22 +277,46 @@ class ModuleJob extends ModuleJobBase { } } - runSync() { + 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()', status, 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(); - const timeout = -1; - const breakOnSigint = false; - setHasStartedUserESMExecution(); - this.module.evaluate(timeout, breakOnSigint); - return { __proto__: null, module: this.module }; + 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 === 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}.`); } async run() { + debug('ModuleJob.run()', this.module); await this.instantiate(); const timeout = -1; const breakOnSigint = false; @@ -262,27 +324,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 }; @@ -351,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; } @@ -371,7 +417,8 @@ class ModuleJobSync extends ModuleJobBase { `Status = ${status}`); } - runSync() { + 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 @@ -380,12 +427,19 @@ 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(); - 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/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 a3362e0bafcb11..355b387e1b7b7a 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; } @@ -648,7 +671,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; } @@ -670,15 +693,14 @@ 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()) { - 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); @@ -720,6 +742,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; @@ -1067,6 +1099,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); @@ -1083,6 +1116,7 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data, target, "createRequiredModuleFacade", CreateRequiredModuleFacade); + SetMethod(isolate, target, "throwIfPromiseRejected", ThrowIfPromiseRejected); } void ModuleWrap::CreatePerContextProperties(Local target, @@ -1122,11 +1156,13 @@ void ModuleWrap::RegisterExternalReferences( registry->Register(GetNamespace); registry->Register(GetStatus); registry->Register(GetError); + registry->Register(IsGraphAsync); registry->Register(CreateRequiredModuleFacade); registry->Register(SetImportModuleDynamicallyCallback); registry->Register(SetInitializeImportMetaObjectCallback); + registry->Register(ThrowIfPromiseRejected); } } // namespace loader } // namespace node 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/src/node_errors.h b/src/node_errors.h index d5e2f86f516bbb..7cacf416b6e624 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, \ @@ -198,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") \ @@ -231,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-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-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/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' }); 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/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()); 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: '' - }); -} 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' }; 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); +// }, +// });