From 3e31baeda6cf597ab3804dd8ca6f049a1d126940 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Fri, 31 Oct 2025 21:45:10 +0100 Subject: [PATCH] esm: use sync loading/resolving on non-loader-hook thread ESM resolution and loading is now always synchronous from a non-loader-hook thread. If no asynchrnous loader hooks are registered, the resolution/loading is entirely synchronous. If asynchronous loader hooks are registered, these would be synchronous on the non-loader-hook thread, and asynchronous on the loader hook thread. This avoids several races caused by async/sync loading sharing the same cache. In particular, asynchronous loader hooks now works with `require(esm)` - previously it tends to break due to races. In addition, when an asynchronous loader hook returns a promise that never settles, the main thread no longer silently exits with exit code 13, leaving the code below any module loading calls silently ignored without being executed. Instead, it now throws ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED which can be caught and handled by the main thread. If the module request comes from `import()`, the never-settling promise is now relayed to the result returned by `import()`. Drive-by: when annotating the error about importing undetectable named exports from CommonJS, it now no longer reload the source code of the CommonJS module, and instead reuses format information cached when the module was loaded for linking. PR-URL: https://github.com/nodejs/node/pull/60380 Fixes: https://github.com/nodejs/node/issues/59666 Reviewed-By: Geoffrey Booth Reviewed-By: Jacob Smith --- doc/api/errors.md | 7 + lib/internal/errors.js | 2 + lib/internal/modules/esm/hooks.js | 28 +- .../modules/esm/initialize_import_meta.js | 3 +- lib/internal/modules/esm/loader.js | 403 ++++++++---------- lib/internal/modules/esm/module_job.js | 231 +++++----- lib/internal/modules/esm/translators.js | 11 +- lib/internal/modules/esm/utils.js | 12 + lib/internal/test_runner/mock/mock.js | 5 +- test/es-module/test-esm-loader-hooks.mjs | 14 +- test/es-module/test-esm-loader-modulemap.js | 9 +- .../import.meta.never-resolve.mjs | 9 +- .../module-hooks/logger-async-hooks.mjs | 19 + .../register-logger-async-hooks.mjs | 2 + .../fixtures/module-hooks/require-esm/cjs.cjs | 1 + .../fixtures/module-hooks/require-esm/esm.mjs | 2 + .../module-hooks/require-esm/inner.cjs | 1 + .../module-hooks/require-esm/inner.mjs | 1 + .../module-hooks/require-esm/main.cjs | 4 + .../module-hooks/require-esm/main.mjs | 3 + .../test-module-hooks-require-esm.js | 72 ++++ 21 files changed, 469 insertions(+), 370 deletions(-) create mode 100644 test/fixtures/module-hooks/logger-async-hooks.mjs create mode 100644 test/fixtures/module-hooks/register-logger-async-hooks.mjs create mode 100644 test/fixtures/module-hooks/require-esm/cjs.cjs create mode 100644 test/fixtures/module-hooks/require-esm/esm.mjs create mode 100644 test/fixtures/module-hooks/require-esm/inner.cjs create mode 100644 test/fixtures/module-hooks/require-esm/inner.mjs create mode 100644 test/fixtures/module-hooks/require-esm/main.cjs create mode 100644 test/fixtures/module-hooks/require-esm/main.mjs create mode 100644 test/module-hooks/test-module-hooks-require-esm.js diff --git a/doc/api/errors.md b/doc/api/errors.md index 8ddc9adf59..2ff3e20625 100644 --- a/doc/api/errors.md +++ b/doc/api/errors.md @@ -703,6 +703,13 @@ by the `node:assert` module. An attempt was made to register something that is not a function as an `AsyncHooks` callback. + + +### `ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED` + +An operation related to module loading is customized by an asynchronous loader +hook that never settled the promise before the loader thread exits. + ### `ERR_ASYNC_TYPE` diff --git a/lib/internal/errors.js b/lib/internal/errors.js index 23a54754e2..5fa4437b09 100644 --- a/lib/internal/errors.js +++ b/lib/internal/errors.js @@ -1137,6 +1137,8 @@ E('ERR_AMBIGUOUS_ARGUMENT', 'The "%s" argument is ambiguous. %s', TypeError); E('ERR_ARG_NOT_ITERABLE', '%s must be iterable', TypeError); E('ERR_ASSERTION', '%s', Error); E('ERR_ASYNC_CALLBACK', '%s must be a function', TypeError); +E('ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED', + 'Async loader request never settled', Error); E('ERR_ASYNC_TYPE', 'Invalid name for async "type": %s', TypeError); E('ERR_BROTLI_INVALID_PARAM', '%s is not a valid Brotli parameter', RangeError); E('ERR_BUFFER_OUT_OF_BOUNDS', diff --git a/lib/internal/modules/esm/hooks.js b/lib/internal/modules/esm/hooks.js index cc66d47a43..ad5a228991 100644 --- a/lib/internal/modules/esm/hooks.js +++ b/lib/internal/modules/esm/hooks.js @@ -23,16 +23,15 @@ const { } = globalThis; const { + ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED, ERR_INTERNAL_ASSERTION, ERR_INVALID_ARG_TYPE, ERR_INVALID_ARG_VALUE, ERR_INVALID_RETURN_PROPERTY_VALUE, ERR_INVALID_RETURN_VALUE, ERR_LOADER_CHAIN_INCOMPLETE, - ERR_METHOD_NOT_IMPLEMENTED, ERR_WORKER_UNSERIALIZABLE_ERROR, } = require('internal/errors').codes; -const { exitCodes: { kUnsettledTopLevelAwait } } = internalBinding('errors'); const { URLParse } = require('internal/url'); const { canParse: URLCanParse } = internalBinding('url'); const { receiveMessageOnPort } = require('worker_threads'); @@ -117,15 +116,16 @@ function defineImportAssertionAlias(context) { * via `ModuleLoader.#setAsyncLoaderHooks()`. * @typedef {object} AsyncLoaderHooks * @property {boolean} allowImportMetaResolve Whether to allow the use of `import.meta.resolve`. + * @property {boolean} isForAsyncLoaderHookWorker Whether the instance is running on the loader hook worker thread. * @property {(url: string, context: object, defaultLoad: Function) => Promise} load * Calling the asynchronous `load` hook asynchronously. - * @property {(url: string, context: object, defaultLoad: Function) => LoadResult} loadSync + * @property {(url: string, context: object, defaultLoad: Function) => LoadResult} [loadSync] * Calling the asynchronous `load` hook synchronously. * @property {(originalSpecifier: string, parentURL: string, * importAttributes: Record) => Promise} resolve * Calling the asynchronous `resolve` hook asynchronously. * @property {(originalSpecifier: string, parentURL: string, - * importAttributes: Record) => ResolveResult} resolveSync + * importAttributes: Record) => ResolveResult} [resolveSync] * Calling the asynchronous `resolve` hook synchronously. * @property {(specifier: string, parentURL: string) => any} register Register asynchronous loader hooks * @property {() => void} waitForLoaderHookInitialization Force loading of hooks. @@ -169,6 +169,8 @@ class AsyncLoaderHooksOnLoaderHookWorker { allowImportMetaResolve = false; + isForAsyncLoaderHookWorker = true; + /** * Import and register custom/user-defined module loader hook(s). * @param {string} urlOrSpecifier @@ -350,10 +352,6 @@ class AsyncLoaderHooksOnLoaderHookWorker { }; } - resolveSync(_originalSpecifier, _parentURL, _importAttributes) { - throw new ERR_METHOD_NOT_IMPLEMENTED('resolveSync()'); - } - /** * Provide source that is understood by one of Node's translators. * @@ -560,7 +558,10 @@ class AsyncLoaderHookWorker { debug('wait for signal from worker'); AtomicsWait(this.#lock, WORKER_TO_MAIN_THREAD_NOTIFICATION, 0); const response = this.#worker.receiveMessageSync(); - if (response == null || response.message.status === 'exit') { return; } + if (response == null) { return; } + if (response.message.status === 'exit') { + process.exit(response.message.body); + } // ! This line catches initialization errors in the worker thread. this.#unwrapMessage(response); @@ -647,10 +648,13 @@ class AsyncLoaderHookWorker { this.#workerNotificationLastId = AtomicsLoad(this.#lock, WORKER_TO_MAIN_THREAD_NOTIFICATION); response = this.#worker.receiveMessageSync(); + debug('got sync message from worker', { method, args, response }); } while (response == null); - debug('got sync response from worker', { method, args }); + if (response.message.status === 'never-settle') { - process.exit(kUnsettledTopLevelAwait); + const error = new ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED(); + error.details = { method, args }; + throw error; } else if (response.message.status === 'exit') { process.exit(response.message.body); } @@ -819,6 +823,8 @@ class AsyncLoaderHooksProxiedToLoaderHookWorker { allowImportMetaResolve = true; + isForAsyncLoaderHookWorker = false; + /** * Instantiate a module loader that uses user-provided custom loader hooks. */ diff --git a/lib/internal/modules/esm/initialize_import_meta.js b/lib/internal/modules/esm/initialize_import_meta.js index 82edb59c41..9646c7c8b6 100644 --- a/lib/internal/modules/esm/initialize_import_meta.js +++ b/lib/internal/modules/esm/initialize_import_meta.js @@ -33,7 +33,8 @@ function createImportMetaResolve(defaultParentURL, loader, allowParentURL) { } try { - ({ url } = loader.resolveSync(specifier, parentURL)); + const request = { specifier, __proto__: null }; + ({ url } = loader.resolveSync(parentURL, request)); return url; } catch (error) { switch (error?.code) { diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index d536d8215c..abfe88c272 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -7,6 +7,8 @@ const { FunctionPrototypeCall, JSONStringify, ObjectSetPrototypeOf, + Promise, + PromisePrototypeThen, RegExpPrototypeSymbolReplace, encodeURIComponent, hardenRegExp, @@ -25,17 +27,20 @@ const { ERR_REQUIRE_ASYNC_MODULE, ERR_REQUIRE_CYCLE_MODULE, ERR_REQUIRE_ESM, - ERR_NETWORK_IMPORT_DISALLOWED, ERR_UNKNOWN_MODULE_FORMAT, } = require('internal/errors').codes; const { getOptionValue } = require('internal/options'); -const { isURL, pathToFileURL, URLParse } = require('internal/url'); +const { isURL, pathToFileURL } = require('internal/url'); const { kEmptyObject } = require('internal/util'); const { compileSourceTextModule, getDefaultConditions, shouldSpawnLoaderHookWorker, + requestTypes: { kImportInRequiredESM, kRequireInImportedCJS, kImportInImportedESM }, } = require('internal/modules/esm/utils'); +/** + * @typedef {import('./utils.js').ModuleRequestType} ModuleRequestType + */ const { kImplicitTypeAttribute } = require('internal/modules/esm/assert'); const { ModuleWrap, @@ -111,15 +116,16 @@ function getTranslators() { * async linking. * @param {string} filename Filename of the module being required. * @param {string|undefined} parentFilename Filename of the module calling require(). + * @param {boolean} isForAsyncLoaderHookWorker Whether this is for the async loader hook worker. * @returns {string} Error message. */ -function getRaceMessage(filename, parentFilename) { - let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `; +function getRaceMessage(filename, parentFilename, isForAsyncLoaderHookWorker) { + let raceMessage = `Cannot require() ES Module ${filename} because it is not yet fully loaded.\n`; 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})`; - } + raceMessage += 'import()-ed via Promise.all().\n'; + raceMessage += 'Try await-ing the import() sequentially in a loop instead.\n'; + raceMessage += ` (From ${parentFilename ? `${parentFilename} in ` : ' '}`; + raceMessage += `${isForAsyncLoaderHookWorker ? 'loader hook worker thread' : 'non-loader-hook thread'})`; return raceMessage; } @@ -184,6 +190,12 @@ class ModuleLoader { */ allowImportMetaResolve; + /** + * @see {AsyncLoaderHooks.isForAsyncLoaderHookWorker} + * Shortcut to this.#asyncLoaderHooks.isForAsyncLoaderHookWorker. + */ + isForAsyncLoaderHookWorker = false; + /** * Asynchronous loader hooks to pass requests to. * @@ -223,8 +235,10 @@ class ModuleLoader { this.#asyncLoaderHooks = asyncLoaderHooks; if (asyncLoaderHooks) { this.allowImportMetaResolve = asyncLoaderHooks.allowImportMetaResolve; + this.isForAsyncLoaderHookWorker = asyncLoaderHooks.isForAsyncLoaderHookWorker; } else { this.allowImportMetaResolve = true; + this.isForAsyncLoaderHookWorker = false; } } @@ -249,7 +263,7 @@ class ModuleLoader { async executeModuleJob(url, wrap, isEntryPoint = false) { const { ModuleJob } = require('internal/modules/esm/module_job'); const module = await onImport.tracePromise(async () => { - const job = new ModuleJob(this, url, undefined, wrap, kEvaluationPhase, false, false); + const job = new ModuleJob(this, url, undefined, wrap, kEvaluationPhase, false, false, kImportInImportedESM); this.loadCache.set(url, undefined, job); const { module } = await job.run(isEntryPoint); return module; @@ -279,62 +293,6 @@ class ModuleLoader { return this.executeModuleJob(url, wrap, isEntryPoint); } - /** - * Get a (possibly not yet fully linked) module job from the cache, or create one and return its Promise. - * @param {string} specifier The module request of the module to be resolved. Typically, what's - * requested by `import ''` or `import('')`. - * @param {string} [parentURL] The URL of the module where the module request is initiated. - * It's undefined if it's from the root module. - * @param {ImportAttributes} importAttributes Attributes from the import statement or expression. - * @param {number} phase Import phase. - * @returns {Promise} - */ - async getModuleJobForImport(specifier, parentURL, importAttributes, phase) { - const resolveResult = await this.resolve(specifier, parentURL, importAttributes); - return this.#getJobFromResolveResult(resolveResult, parentURL, importAttributes, phase, false); - } - - /** - * Similar to {@link getModuleJobForImport} but it's used for `require()` resolved by the ESM loader - * in imported CJS modules. This runs synchronously and when it returns, the module job's module - * requests are all linked. - * @param {string} specifier See {@link getModuleJobForImport} - * @param {string} [parentURL] See {@link getModuleJobForImport} - * @param {ImportAttributes} importAttributes See {@link getModuleJobForImport} - * @param {number} phase Import phase. - * @returns {Promise} - */ - getModuleJobForRequireInImportedCJS(specifier, parentURL, importAttributes, phase) { - const resolveResult = this.resolveSync(specifier, parentURL, importAttributes); - return this.#getJobFromResolveResult(resolveResult, parentURL, importAttributes, phase, true); - } - - /** - * Given a resolved module request, obtain a ModuleJobBase from it - if it's already cached, - * return the cached ModuleJobBase. Otherwise, load its source and translate it into a ModuleWrap first. - * @param {{ format: string, url: string }} resolveResult Resolved module request. - * @param {string} [parentURL] See {@link getModuleJobForImport} - * @param {ImportAttributes} importAttributes See {@link getModuleJobForImport} - * @param {number} phase Import phase. - * @param {boolean} isForRequireInImportedCJS Whether this is done for require() in imported CJS. - * @returns {ModuleJobBase} - */ - #getJobFromResolveResult(resolveResult, parentURL, importAttributes, phase, - isForRequireInImportedCJS = false) { - const { url, format } = resolveResult; - const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes; - let job = this.loadCache.get(url, resolvedImportAttributes.type); - - if (job === undefined) { - job = this.#createModuleJob(url, resolvedImportAttributes, phase, parentURL, format, - isForRequireInImportedCJS); - } else { - job.ensurePhase(phase); - } - - return job; - } - /** * This constructs (creates, instantiates and evaluates) a module graph that * is require()'d. @@ -347,6 +305,10 @@ class ModuleLoader { */ importSyncForRequire(mod, filename, source, isMain, parent) { const url = pathToFileURL(filename).href; + if (!getOptionValue('--experimental-require-module')) { + throw new ERR_REQUIRE_ESM(url, true); + } + let job = this.loadCache.get(url, kImplicitTypeAttribute); // This module job is already created: // 1. If it was loaded by `require()` before, at this point the instantiation @@ -364,9 +326,9 @@ class ModuleLoader { 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. + // This race should only be possible on the loader hook thread. See https://github.com/nodejs/node/issues/59666 if (!job.module) { - assert.fail(getRaceMessage(filename, parentFilename)); + assert.fail(getRaceMessage(filename, parentFilename), this.isForAsyncLoaderHookWorker); } const status = job.module.getStatus(); debug('Module status', job, status); @@ -419,92 +381,67 @@ class ModuleLoader { const inspectBrk = (isMain && getOptionValue('--inspect-brk')); const { ModuleJobSync } = require('internal/modules/esm/module_job'); - job = new ModuleJobSync(this, url, kEmptyObject, wrap, kEvaluationPhase, isMain, inspectBrk); + job = new ModuleJobSync(this, url, kEmptyObject, wrap, kEvaluationPhase, isMain, inspectBrk, + kImportInRequiredESM); this.loadCache.set(url, kImplicitTypeAttribute, job); mod[kRequiredModuleSymbol] = job.module; return { wrap: job.module, namespace: job.runSync(parent).namespace }; } /** - * Resolve individual module requests and create or get the cached ModuleWraps for - * each of them. This is only used to create a module graph being require()'d. - * @param {string} specifier Specifier of the the imported module. - * @param {string} parentURL Where the import comes from. - * @param {object} importAttributes import attributes from the import statement. - * @param {number} phase The import phase. - * @returns {ModuleJobBase} + * Check invariants on a cached module job when require()'d from ESM. + * @param {string} specifier The first parameter of require(). + * @param {string} url URL of the module being required. + * @param {string|undefined} parentURL URL of the module calling require(). + * @param {ModuleJobBase} job The cached module job. */ - getModuleJobForRequire(specifier, parentURL, importAttributes, phase) { - const parsed = URLParse(specifier); - if (parsed != null) { - const protocol = parsed.protocol; - if (protocol === 'https:' || protocol === 'http:') { - throw new ERR_NETWORK_IMPORT_DISALLOWED(specifier, parentURL, - 'ES modules cannot be loaded by require() from the network'); + #checkCachedJobForRequireESM(specifier, url, parentURL, job) { + // This race should only be possible on the loader hook thread. See https://github.com/nodejs/node/issues/59666 + if (!job.module) { + assert.fail(getRaceMessage(url, parentURL, this.isForAsyncLoaderHookWorker)); + } + // This module is being evaluated, which means it's imported in a previous link + // in a cycle. + if (job.module.getStatus() === kEvaluating) { + const parentFilename = urlToFilename(parentURL); + let message = `Cannot import Module ${specifier} in a cycle.`; + if (parentFilename) { + message += ` (from ${parentFilename})`; } - assert(protocol === 'file:' || protocol === 'node:' || protocol === 'data:'); + throw new ERR_REQUIRE_CYCLE_MODULE(message); } - // TODO(joyeecheung): consolidate cache behavior and use resolveSync() and - // loadSync() here. - const resolveResult = this.#cachedResolveSync(specifier, parentURL, importAttributes); - const { url, format } = resolveResult; - if (!getOptionValue('--experimental-require-module')) { - throw new ERR_REQUIRE_ESM(url, true); - } - const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes; - let job = this.loadCache.get(url, resolvedImportAttributes.type); - if (job !== undefined) { - // TODO(node:55782): this race may stop happening when the ESM resolution and loading become synchronous. - if (!job.module) { - assert.fail(getRaceMessage(url, parentURL)); - } - // This module is being evaluated, which means it's imported in a previous link - // in a cycle. - if (job.module.getStatus() === kEvaluating) { - const parentFilename = urlToFilename(parentURL); - let message = `Cannot import Module ${specifier} in a cycle.`; - if (parentFilename) { - message += ` (from ${parentFilename})`; - } - throw new ERR_REQUIRE_CYCLE_MODULE(message); - } - job.ensurePhase(phase); - // Otherwise the module could be imported before but the evaluation may be already - // completed (e.g. the require call is lazy) so it's okay. We will return the - // module now and check asynchronicity of the entire graph later, after the - // graph is instantiated. - return job; - } + // Otherwise the module could be imported before but the evaluation may be already + // completed (e.g. the require call is lazy) so it's okay. We will return the + // job and check asynchronicity of the entire graph later, after the + // graph is instantiated. + } - const loadResult = this.#loadSync(url, { format, importAttributes }); - - const formatFromLoad = loadResult.format; + /** + * Load a module and translate it into a ModuleWrap for require(esm). + * This is run synchronously, and the translator always return a ModuleWrap synchronously. + * @param {string} url URL of the module to be translated. + * @param {object} loadContext See {@link load} + * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point. + * @param {ModuleRequest} request Module request. + * @returns {ModuleWrap} + */ + loadAndTranslateForImportInRequiredESM(url, loadContext, parentURL, request) { + const loadResult = this.#loadSync(url, loadContext); // Use the synchronous commonjs translator which can deal with cycles. + const formatFromLoad = loadResult.format; const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ? 'commonjs-sync' : formatFromLoad; - - if (translatorKey === 'wasm') { - assert.fail('WASM is currently unsupported by require(esm)'); - } - - const { source } = loadResult; - const isMain = (parentURL === undefined); - const translateContext = { format: formatFromLoad, source, translatorKey, __proto__: null }; + const translateContext = { ...loadResult, translatorKey, __proto__: null }; const wrap = this.#translate(url, translateContext, parentURL); assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`); - if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { - process.send({ 'watch:import': [url] }); - } - const cjsModule = wrap[imported_cjs_symbol]; if (cjsModule) { - assert(translatorKey === 'commonjs-sync'); // Check if the ESM initiating import CJS is being required by the same CJS module. if (cjsModule?.[kIsExecuting]) { const parentFilename = urlToFilename(parentURL); - let message = `Cannot import CommonJS Module ${specifier} in a cycle.`; + let message = `Cannot import CommonJS Module ${request.specifier} in a cycle.`; if (parentFilename) { message += ` (from ${parentFilename})`; } @@ -512,12 +449,7 @@ class ModuleLoader { } } - const inspectBrk = (isMain && getOptionValue('--inspect-brk')); - const { ModuleJobSync } = require('internal/modules/esm/module_job'); - job = new ModuleJobSync(this, url, importAttributes, wrap, phase, isMain, inspectBrk); - - this.loadCache.set(url, importAttributes.type, job); - return job; + return wrap; } /** @@ -540,6 +472,9 @@ class ModuleLoader { const result = FunctionPrototypeCall(translator, this, url, translateContext, parentURL); assert(result instanceof ModuleWrap, `The ${format} module returned is not a ModuleWrap`); + if (format === 'commonjs' || format === 'commonjs-sync' || format === 'require-commonjs') { + result.isCommonJS = true; + } return result; } @@ -594,54 +529,72 @@ class ModuleLoader { return this.#translate(url, translateContext, parentURL); }; if (isPromise(maybePromise)) { - return maybePromise.then(afterLoad); + return PromisePrototypeThen(maybePromise, afterLoad); } return afterLoad(maybePromise); } /** - * Load a module and translate it into a ModuleWrap, and create a ModuleJob from it. - * This runs synchronously. If isForRequireInImportedCJS is true, the module should be linked - * by the time this returns. Otherwise it may still have pending module requests. - * @param {string} url The URL that was resolved for this module. - * @param {ImportAttributes} importAttributes See {@link getModuleJobForImport} - * @param {number} phase Import phase. - * @param {string} [parentURL] See {@link getModuleJobForImport} - * @param {string} [format] The format hint possibly returned by the `resolve` hook - * @param {boolean} isForRequireInImportedCJS Whether this module job is created for require() - * in imported CJS. + * Given a resolved module request, obtain a ModuleJobBase from it - if it's already cached, + * return the cached ModuleJobBase. Otherwise, load its source and translate it into a ModuleWrap first. + * This runs synchronously. On any thread that is not an async loader hook worker thread, + * the module should be linked by the time this returns. Otherwise it may still have + * pending module requests. + * @param {string} parentURL See {@link getOrCreateModuleJob} + * @param {{format: string, url: string}} resolveResult + * @param {ModuleRequest} request Module request. + * @param {ModuleRequestType} requestType Type of the module request. * @returns {ModuleJobBase} The (possibly pending) module job */ - #createModuleJob(url, importAttributes, phase, parentURL, format, isForRequireInImportedCJS) { - const context = { format, importAttributes }; + #getOrCreateModuleJobAfterResolve(parentURL, resolveResult, request, requestType) { + const { url, format } = resolveResult; + // TODO(joyeecheung): update the module requests to use importAttributes as property names. + const importAttributes = resolveResult.importAttributes ?? request.attributes; + let job = this.loadCache.get(url, importAttributes.type); + + if (job !== undefined) { + if (requestType === kImportInRequiredESM) { + this.#checkCachedJobForRequireESM(request.specifier, url, parentURL, job); + } + job.ensurePhase(request.phase, requestType); + return job; + } + + const context = { format, importAttributes, __proto__: null }; - const isMain = parentURL === undefined; let moduleOrModulePromise; - if (isForRequireInImportedCJS) { + if (requestType === kRequireInImportedCJS) { moduleOrModulePromise = this.loadAndTranslateForRequireInImportedCJS(url, context, parentURL); + } else if (requestType === kImportInRequiredESM) { + moduleOrModulePromise = this.loadAndTranslateForImportInRequiredESM(url, context, parentURL, request); } else { moduleOrModulePromise = this.loadAndTranslate(url, context, parentURL); } - const inspectBrk = ( - isMain && - getOptionValue('--inspect-brk') - ); - - if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { - process.send({ 'watch:import': [url] }); + if (requestType === kImportInRequiredESM || requestType === kRequireInImportedCJS || + !this.isForAsyncLoaderHookWorker) { + assert(moduleOrModulePromise instanceof ModuleWrap, `Expected ModuleWrap for loading ${url}`); } - const { ModuleJob } = require('internal/modules/esm/module_job'); - const job = new ModuleJob( + if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { + const type = requestType === kRequireInImportedCJS ? 'require' : 'import'; + process.send({ [`watch:${type}`]: [url] }); + } + + const { ModuleJob, ModuleJobSync } = require('internal/modules/esm/module_job'); + // TODO(joyeecheung): use ModuleJobSync for kRequireInImportedCJS too. + const ModuleJobCtor = (requestType === kImportInRequiredESM ? ModuleJobSync : ModuleJob); + const isMain = (parentURL === undefined); + const inspectBrk = (isMain && getOptionValue('--inspect-brk')); + job = new ModuleJobCtor( this, url, importAttributes, moduleOrModulePromise, - phase, + request.phase, isMain, inspectBrk, - isForRequireInImportedCJS, + requestType, ); this.loadCache.set(url, importAttributes.type, job); @@ -649,6 +602,34 @@ class ModuleLoader { return job; } + /** + * Get a (possibly not yet fully linked) module job from the cache, or create one and return its Promise. + * @param {string} [parentURL] The URL of the module where the module request is initiated. + * It's undefined if it's from the root module. + * @param {ModuleRequest} request Module request. + * @param {ModuleRequestType} requestType Type of the module request. + * @returns {Promise|ModuleJobBase} + */ + getOrCreateModuleJob(parentURL, request, requestType) { + let maybePromise; + if (requestType === kRequireInImportedCJS || requestType === kImportInRequiredESM) { + // In these two cases, resolution must be synchronous. + maybePromise = this.resolveSync(parentURL, request); + assert(!isPromise(maybePromise)); + } else { + maybePromise = this.#resolve(parentURL, request); + } + + const afterResolve = (resolveResult) => { + return this.#getOrCreateModuleJobAfterResolve(parentURL, resolveResult, request, requestType); + }; + + if (isPromise(maybePromise)) { + return PromisePrototypeThen(maybePromise, afterResolve); + } + return afterResolve(maybePromise); + } + /** * This method is usually called indirectly as part of the loading processes. * Use directly with caution. @@ -662,8 +643,16 @@ class ModuleLoader { */ async import(specifier, parentURL, importAttributes, phase = kEvaluationPhase, isEntryPoint = false) { return onImport.tracePromise(async () => { - const moduleJob = await this.getModuleJobForImport(specifier, parentURL, importAttributes, - phase); + const request = { specifier, phase, attributes: importAttributes, __proto__: null }; + let moduleJob; + try { + moduleJob = await this.getOrCreateModuleJob(parentURL, request); + } catch (e) { + if (e?.code === 'ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED') { + return new Promise(() => {}); + } + throw e; + } if (phase === kSourcePhase) { const module = await moduleJob.modulePromise; return module.getModuleSourceObject(); @@ -695,28 +684,20 @@ class ModuleLoader { /** * Resolve a module request to a URL identifying the location of the module. Handles customization hooks, * if any. - * @param {string|URL} specifier The module request of the module to be resolved. Typically, what's - * requested by `import specifier`, `import(specifier)` or `import.meta.resolve(specifier)`. * @param {string} [parentURL] The URL of the module where the module request is initiated. * It's undefined if it's from the root module. - * @param {ImportAttributes} importAttributes Attributes from the import statement or expression. - * @returns {Promise<{format: string, url: string}>} + * @param {ModuleRequest} request Module request. + * @returns {Promise<{format: string, url: string}>|{format: string, url: string}} */ - resolve(specifier, parentURL, importAttributes) { - specifier = `${specifier}`; - if (syncResolveHooks.length) { - // Has module.registerHooks() hooks, use the synchronous variant that can handle both hooks. - return this.resolveSync(specifier, parentURL, importAttributes); - } - if (this.#asyncLoaderHooks) { // Only has module.register hooks. + #resolve(parentURL, request) { + if (this.isForAsyncLoaderHookWorker) { + const specifier = `${request.specifier}`; + const importAttributes = request.attributes ?? kEmptyObject; + // TODO(joyeecheung): invoke the synchronous hooks in the default step on loader thread. return this.#asyncLoaderHooks.resolve(specifier, parentURL, importAttributes); } - return this.#cachedDefaultResolve(specifier, { - __proto__: null, - conditions: this.#defaultConditions, - parentURL, - importAttributes, - }); + + return this.resolveSync(parentURL, request); } /** @@ -739,25 +720,6 @@ class ModuleLoader { return result; } - /** - * Either return a cached resolution, or perform the synchronous resolution, and - * cache the result. - * @param {string} specifier See {@link resolve}. - * @param {string} [parentURL] See {@link resolve}. - * @param {ImportAttributes} importAttributes See {@link resolve}. - * @returns {{ format: string, url: string }} - */ - #cachedResolveSync(specifier, parentURL, importAttributes) { - const requestKey = this.#resolveCache.serializeKey(specifier, importAttributes); - const cachedResult = this.#resolveCache.get(requestKey, parentURL); - if (cachedResult != null) { - return cachedResult; - } - const result = this.resolveSync(specifier, parentURL, importAttributes); - this.#resolveCache.set(requestKey, parentURL, result); - return result; - } - /** * This is the default resolve step for module.registerHooks(), which incorporates asynchronous hooks * from module.register() which are run in a blocking fashion for it to be synchronous. @@ -767,7 +729,7 @@ class ModuleLoader { * @returns {{ format: string, url: string }} */ #resolveAndMaybeBlockOnLoaderThread(specifier, context) { - if (this.#asyncLoaderHooks) { + if (this.#asyncLoaderHooks?.resolveSync) { return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes); } return this.#cachedDefaultResolve(specifier, context); @@ -779,46 +741,43 @@ class ModuleLoader { * from the loader thread for this to be synchronous. * This is here to support `import.meta.resolve()`, `require()` in imported CJS, and * `module.registerHooks()` hooks. - * - * TODO(joyeecheung): consolidate the cache behavior and use this in require(esm). - * @param {string|URL} specifier See {@link resolve}. * @param {string} [parentURL] See {@link resolve}. - * @param {ImportAttributes} [importAttributes] See {@link resolve}. + * @param {ModuleRequest} request See {@link resolve}. * @returns {{ format: string, url: string }} */ - resolveSync(specifier, parentURL, importAttributes = { __proto__: null }) { - specifier = `${specifier}`; + resolveSync(parentURL, request) { + const specifier = `${request.specifier}`; + const importAttributes = request.attributes ?? kEmptyObject; + if (syncResolveHooks.length) { // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step. return resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions, this.#resolveAndMaybeBlockOnLoaderThread.bind(this)); } - return this.#resolveAndMaybeBlockOnLoaderThread(specifier, { - __proto__: null, + const context = { + ...request, conditions: this.#defaultConditions, parentURL, importAttributes, - }); + __proto__: null, + }; + return this.#resolveAndMaybeBlockOnLoaderThread(specifier, context); } /** * Provide source that is understood by one of Node's translators. Handles customization hooks, * if any. + * @typedef { {format: ModuleFormat, source: ModuleSource }} LoadResult * @param {string} url The URL of the module to be loaded. * @param {object} context Metadata about the module - * @returns {Promise<{ format: ModuleFormat, source: ModuleSource }> | { format: ModuleFormat, source: ModuleSource }} + * @returns {Promise | LoadResult}} */ load(url, context) { - if (syncLoadHooks.length) { - // Has module.registerHooks() hooks, use the synchronous variant that can handle both hooks. - return this.#loadSync(url, context); - } - if (this.#asyncLoaderHooks) { + if (this.isForAsyncLoaderHookWorker) { + // TODO(joyeecheung): invoke the synchronous hooks in the default step on loader thread. return this.#asyncLoaderHooks.load(url, context); } - - defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; - return defaultLoadSync(url, context); + return this.#loadSync(url, context); } /** @@ -829,7 +788,7 @@ class ModuleLoader { * @returns {{ format: ModuleFormat, source: ModuleSource }} */ #loadAndMaybeBlockOnLoaderThread(url, context) { - if (this.#asyncLoaderHooks) { + if (this.#asyncLoaderHooks?.loadSync) { return this.#asyncLoaderHooks.loadSync(url, context); } defaultLoadSync ??= require('internal/modules/esm/load').defaultLoadSync; @@ -841,8 +800,6 @@ class ModuleLoader { * from module.register(), this blocks on the loader thread for it to return synchronously. * * This is here to support `require()` in imported CJS and `module.registerHooks()` hooks. - * - * TODO(joyeecheung): consolidate the cache behavior and use this in require(esm). * @param {string} url See {@link load} * @param {object} [context] See {@link load} * @returns {{ format: ModuleFormat, source: ModuleSource }} diff --git a/lib/internal/modules/esm/module_job.js b/lib/internal/modules/esm/module_job.js index f032b2eeb1..8459ab5f47 100644 --- a/lib/internal/modules/esm/module_job.js +++ b/lib/internal/modules/esm/module_job.js @@ -3,6 +3,7 @@ const { Array, ArrayPrototypeJoin, + ArrayPrototypePush, ArrayPrototypeSome, FunctionPrototype, ObjectSetPrototypeOf, @@ -36,7 +37,10 @@ const { entry_point_module_private_symbol, }, } = internalBinding('util'); -const { decorateErrorStack, kEmptyObject } = require('internal/util'); +/** + * @typedef {import('./utils.js').ModuleRequestType} ModuleRequestType + */ +const { decorateErrorStack } = require('internal/util'); const { isPromise } = require('internal/util/types'); const { getSourceMapsSupport, @@ -106,8 +110,9 @@ const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => { }; class ModuleJobBase { - constructor(url, importAttributes, phase, isMain, inspectBrk) { + constructor(loader, url, importAttributes, phase, isMain, inspectBrk) { assert(typeof phase === 'number'); + this.loader = loader; this.importAttributes = importAttributes; this.phase = phase; this.isMain = isMain; @@ -115,13 +120,65 @@ class ModuleJobBase { this.url = url; } + + /** + * Synchronously link the module and its dependencies. + * @param {ModuleRequestType} requestType Type of the module request. + * @returns {ModuleJobBase[]} + */ + syncLink(requestType) { + // Store itself into the cache first before linking in case there are circular + // references in the linking. + this.loader.loadCache.set(this.url, this.type, this); + const moduleRequests = this.module.getModuleRequests(); + // Modules should be aligned with the moduleRequests array in order. + const modules = Array(moduleRequests.length); + const evaluationDepJobs = []; + this.commonJsDeps = Array(moduleRequests.length); + try { + for (let idx = 0; idx < moduleRequests.length; idx++) { + const request = moduleRequests[idx]; + // TODO(joyeecheung): split this into two iterators, one for resolving and one for loading so + // that hooks can pre-fetch sources off-thread. + const job = this.loader.getOrCreateModuleJob(this.url, request, requestType); + debug(`ModuleJobBase.syncLink() ${this.url} -> ${request.specifier}`, job); + assert(!isPromise(job)); + assert(job.module instanceof ModuleWrap); + if (request.phase === kEvaluationPhase) { + ArrayPrototypePush(evaluationDepJobs, job); + } + modules[idx] = job.module; + this.commonJsDeps[idx] = job.module.isCommonJS; + } + this.module.link(modules); + } finally { + // Restore it - if it succeeds, we'll reset in the caller; Otherwise it's + // not cached and if the error is caught, subsequent attempt would still fail. + this.loader.loadCache.delete(this.url, this.type); + } + + return evaluationDepJobs; + } + + /** + * Ensure that this ModuleJob is moving towards the required phase + * (does not necessarily mean it is ready at that phase - run does that) + * @param {number} phase + */ + ensurePhase(phase, requestType) { + if (this.phase < phase) { + this.phase = phase; + this.linked = this.link(requestType); + if (isPromise(this.linked)) { + PromisePrototypeThen(this.linked, undefined, noop); + } + } + } } /* A ModuleJob tracks the loading of a single Module, and the ModuleJobs of * its dependencies, over time. */ class ModuleJob extends ModuleJobBase { - #loader = null; - /** * @param {ModuleLoader} loader The ESM loader. * @param {string} url URL of the module to be wrapped in ModuleJob. @@ -131,12 +188,11 @@ class ModuleJob extends ModuleJobBase { * @param {boolean} isMain Whether the module is the entry point. * @param {boolean} inspectBrk Whether this module should be evaluated with the * first line paused in the debugger (because --inspect-brk is passed). - * @param {boolean} isForRequireInImportedCJS Whether this is created for require() in imported CJS. + * @param {ModuleRequestType} requestType Type of the module request. */ constructor(loader, url, importAttributes = { __proto__: null }, moduleOrModulePromise, - phase = kEvaluationPhase, isMain, inspectBrk, isForRequireInImportedCJS = false) { - super(url, importAttributes, phase, isMain, inspectBrk); - this.#loader = loader; + phase = kEvaluationPhase, isMain, inspectBrk, requestType) { + super(loader, url, importAttributes, phase, isMain, inspectBrk); // Expose the promise to the ModuleWrap directly for linking below. if (isPromise(moduleOrModulePromise)) { @@ -148,10 +204,12 @@ class ModuleJob extends ModuleJobBase { if (this.phase === kEvaluationPhase) { // Promise for the list of all dependencyJobs. - this.linked = this.#link(); + this.linked = this.link(requestType); // This promise is awaited later anyway, so silence // 'unhandled rejection' warnings. - PromisePrototypeThen(this.linked, undefined, noop); + if (isPromise(this.linked)) { + PromisePrototypeThen(this.linked, undefined, noop); + } } // instantiated == deep dependency jobs wrappers are instantiated, @@ -160,79 +218,69 @@ class ModuleJob extends ModuleJobBase { } /** - * Ensure that this ModuleJob is moving towards the required phase - * (does not necessarily mean it is ready at that phase - run does that) - * @param {number} phase + * @param {ModuleRequestType} requestType Type of the module request. + * @returns {ModuleJobBase[]|Promise} */ - ensurePhase(phase) { - if (this.phase < phase) { - this.phase = phase; - this.linked = this.#link(); - PromisePrototypeThen(this.linked, undefined, noop); + link(requestType) { + if (this.loader.isForAsyncLoaderHookWorker) { + return this.#asyncLink(requestType); } + return this.syncLink(requestType); } /** - * Iterates the module requests and links with the loader. - * @returns {Promise} Dependency module jobs. + * @param {ModuleRequestType} requestType Type of the module request. + * @returns {Promise} */ - async #link() { + async #asyncLink(requestType) { + assert(this.loader.isForAsyncLoaderHookWorker); this.module = await this.modulePromise; assert(this.module instanceof ModuleWrap); - const moduleRequests = this.module.getModuleRequests(); - // Explicitly keeping track of dependency jobs is needed in order - // to flatten out the dependency graph below in `_instantiate()`, - // so that circular dependencies can't cause a deadlock by two of - // these `link` callbacks depending on each other. // Create an ArrayLike to avoid calling into userspace with `.then` // when returned from the async function. - const evaluationDepJobs = Array(moduleRequests.length); - ObjectSetPrototypeOf(evaluationDepJobs, null); - // Modules should be aligned with the moduleRequests array in order. const modulePromises = Array(moduleRequests.length); - // Track each loop for whether it is an evaluation phase or source phase request. - let isEvaluation; - // Iterate with index to avoid calling into userspace with `Symbol.iterator`. - for ( - let idx = 0, eidx = 0; - // Use the let-scoped eidx value to update the executionDepJobs length at the end of the loop. - idx < moduleRequests.length || (evaluationDepJobs.length = eidx, false); - idx++, eidx += isEvaluation - ) { - const { specifier, phase, attributes } = moduleRequests[idx]; - isEvaluation = phase === kEvaluationPhase; - // TODO(joyeecheung): resolve all requests first, then load them in another - // loop so that hooks can pre-fetch sources off-thread. - const dependencyJobPromise = this.#loader.getModuleJobForImport( - specifier, this.url, attributes, phase, - ); + const evaluationDepJobs = []; + this.commonJsDeps = Array(moduleRequests.length); + for (let idx = 0; idx < moduleRequests.length; idx++) { + const request = moduleRequests[idx]; + // Explicitly keeping track of dependency jobs is needed in order + // to flatten out the dependency graph below in `asyncInstantiate()`, + // so that circular dependencies can't cause a deadlock by two of + // these `link` callbacks depending on each other. + // TODO(joyeecheung): split this into two iterators, one for resolving and one for loading so + // that hooks can pre-fetch sources off-thread. + const dependencyJobPromise = this.loader.getOrCreateModuleJob(this.url, request, requestType); const modulePromise = PromisePrototypeThen(dependencyJobPromise, (job) => { - debug(`async link() ${this.url} -> ${specifier}`, job); - if (phase === kEvaluationPhase) { - evaluationDepJobs[eidx] = job; + debug(`ModuleJob.asyncLink() ${this.url} -> ${request.specifier}`, job); + if (request.phase === kEvaluationPhase) { + ArrayPrototypePush(evaluationDepJobs, job); } return job.modulePromise; }); modulePromises[idx] = modulePromise; } - const modules = await SafePromiseAllReturnArrayLike(modulePromises); - this.module.link(modules); + for (let idx = 0; idx < moduleRequests.length; idx++) { + this.commonJsDeps[idx] = modules[idx].isCommonJS; + } + this.module.link(modules); return evaluationDepJobs; } #instantiate() { if (this.instantiated === undefined) { - this.instantiated = this.#_instantiate(); + this.instantiated = this.#asyncInstantiate(); } return this.instantiated; } - async #_instantiate() { + async #asyncInstantiate() { const jobsInGraph = new SafeSet(); + // TODO(joyeecheung): if it's not on the async loader thread, consider this already + // linked. const addJobsToDependencyGraph = async (moduleJob) => { debug(`async addJobsToDependencyGraph() ${this.url}`, moduleJob); @@ -240,7 +288,7 @@ class ModuleJob extends ModuleJobBase { return; } jobsInGraph.add(moduleJob); - const dependencyJobs = await moduleJob.linked; + const dependencyJobs = isPromise(moduleJob.linked) ? await moduleJob.linked : moduleJob.linked; return SafePromiseAllReturnVoid(dependencyJobs, addJobsToDependencyGraph); }; await addJobsToDependencyGraph(this); @@ -263,31 +311,19 @@ class ModuleJob extends ModuleJobBase { StringPrototypeIncludes(e.message, ' does not provide an export named')) { const splitStack = StringPrototypeSplit(e.stack, '\n', 2); - const parentFileUrl = RegExpPrototypeSymbolReplace( - /:\d+$/, - splitStack[0], - '', - ); const { 1: childSpecifier, 2: name } = RegExpPrototypeExec( /module '(.*)' does not provide an export named '(.+)'/, e.message); - const { url: childFileURL } = await this.#loader.resolve( - childSpecifier, - parentFileUrl, - kEmptyObject, - ); - let format; - try { - // This might throw for non-CommonJS modules because we aren't passing - // in the import attributes and some formats require them; but we only - // care about CommonJS for the purposes of this error message. - ({ format } = - await this.#loader.load(childFileURL)); - } catch { - // Continue regardless of error. + const moduleRequests = this.module.getModuleRequests(); + let isCommonJS = false; + for (let i = 0; i < moduleRequests.length; ++i) { + if (moduleRequests[i].specifier === childSpecifier) { + isCommonJS = this.commonJsDeps[i]; + break; + } } - if (format === 'commonjs') { + if (isCommonJS) { const importStatement = splitStack[1]; // TODO(@ctavan): The original error stack only provides the single // line which causes the error. For multi-line import statements we @@ -394,8 +430,6 @@ class ModuleJob extends ModuleJobBase { * TODO(joyeecheung): consolidate this with the isForRequireInImportedCJS variant of ModuleJob. */ class ModuleJobSync extends ModuleJobBase { - #loader = null; - /** * @param {ModuleLoader} loader The ESM loader. * @param {string} url URL of the module to be wrapped in ModuleJob. @@ -407,57 +441,26 @@ class ModuleJobSync extends ModuleJobBase { * first line paused in the debugger (because --inspect-brk is passed). */ constructor(loader, url, importAttributes, moduleWrap, phase = kEvaluationPhase, isMain, - inspectBrk) { - super(url, importAttributes, phase, isMain, inspectBrk, true); + inspectBrk, requestType) { + super(loader, url, importAttributes, phase, isMain, inspectBrk); - this.#loader = loader; this.module = moduleWrap; assert(this.module instanceof ModuleWrap); this.linked = undefined; this.type = importAttributes.type; if (phase === kEvaluationPhase) { - this.#link(); + this.linked = this.link(requestType); } } /** - * Ensure that this ModuleJob is at the required phase - * @param {number} phase + * @param {ModuleRequestType} requestType Type of the module request. + * @returns {ModuleJobBase[]} */ - ensurePhase(phase) { - if (this.phase < phase) { - this.phase = phase; - this.#link(); - } - } - - #link() { - // Store itself into the cache first before linking in case there are circular - // references in the linking. - this.#loader.loadCache.set(this.url, this.type, this); - try { - const moduleRequests = this.module.getModuleRequests(); - // Modules should be aligned with the moduleRequests array in order. - const modules = Array(moduleRequests.length); - const evaluationDepJobs = Array(moduleRequests.length); - let j = 0; - for (let i = 0; i < moduleRequests.length; ++i) { - const { specifier, attributes, phase } = moduleRequests[i]; - const job = this.#loader.getModuleJobForRequire(specifier, this.url, attributes, phase); - modules[i] = job.module; - if (phase === kEvaluationPhase) { - evaluationDepJobs[j++] = job; - } - } - evaluationDepJobs.length = j; - this.module.link(modules); - this.linked = evaluationDepJobs; - } finally { - // Restore it - if it succeeds, we'll reset in the caller; Otherwise it's - // not cached and if the error is caught, subsequent attempt would still fail. - this.#loader.loadCache.delete(this.url, this.type); - } + link(requestType) { + // Synchronous linking is always used for ModuleJobSync. + return this.syncLink(requestType); } get modulePromise() { diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 446349113e..1716328c7a 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -57,7 +57,7 @@ const { } = require('internal/errors').codes; const { maybeCacheSourceMap } = require('internal/source_map/source_map_cache'); const moduleWrap = internalBinding('module_wrap'); -const { ModuleWrap } = moduleWrap; +const { ModuleWrap, kEvaluationPhase } = moduleWrap; // Lazy-loading to avoid circular dependencies. let getSourceSync; @@ -112,6 +112,7 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU return module; }); +const { requestTypes: { kRequireInImportedCJS } } = require('internal/modules/esm/utils'); /** * Loads a CommonJS module via the ESM Loader sync CommonJS translator. * This translator creates its own version of the `require` function passed into CommonJS modules. @@ -131,7 +132,6 @@ function loadCJSModule(module, source, url, filename, isMain) { if (sourceMapURL) { maybeCacheSourceMap(url, source, module, false, sourceURL, sourceMapURL); } - const cascadedLoader = require('internal/modules/esm/loader').getOrInitializeCascadedLoader(); const __dirname = dirname(filename); // eslint-disable-next-line func-name-matching,func-style @@ -154,7 +154,8 @@ function loadCJSModule(module, source, url, filename, isMain) { // FIXME(node:59666) Currently, the ESM loader re-invents require() here for imported CJS and this // requires a separate cache to be populated as well as introducing several quirks. This is not ideal. - const job = cascadedLoader.getModuleJobForRequireInImportedCJS(specifier, url, importAttributes); + const request = { specifier, attributes: importAttributes, phase: kEvaluationPhase, __proto__: null }; + const job = cascadedLoader.getOrCreateModuleJob(url, request, kRequireInImportedCJS); job.runSync(); let mod = cjsCache.get(job.url); assert(job.module, `Imported CJS module ${url} failed to load module ${job.url} using require() due to race condition`); @@ -185,7 +186,9 @@ function loadCJSModule(module, source, url, filename, isMain) { specifier = `${pathToFileURL(path)}`; } } - const { url: resolvedURL } = cascadedLoader.resolveSync(specifier, url, kEmptyObject); + + const request = { specifier, __proto__: null, attributes: kEmptyObject }; + const { url: resolvedURL } = cascadedLoader.resolveSync(url, request); return urlToFilename(resolvedURL); }); setOwnProperty(requireFn, 'main', process.mainModule); diff --git a/lib/internal/modules/esm/utils.js b/lib/internal/modules/esm/utils.js index a9076a7ae9..0af25ebbf6 100644 --- a/lib/internal/modules/esm/utils.js +++ b/lib/internal/modules/esm/utils.js @@ -5,6 +5,7 @@ const { ObjectFreeze, SafeSet, SafeWeakMap, + Symbol, } = primordials; const { @@ -332,6 +333,16 @@ function compileSourceTextModule(url, source, cascadedLoader, context = kEmptyOb return wrap; } + +const kImportInImportedESM = Symbol('kImportInImportedESM'); +const kImportInRequiredESM = Symbol('kImportInRequiredESM'); +const kRequireInImportedCJS = Symbol('kRequireInImportedCJS'); + +/** + * @typedef {kImportInImportedESM | kImportInRequiredESM | kRequireInImportedCJS} ModuleRequestType + */ +const requestTypes = { kImportInImportedESM, kImportInRequiredESM, kRequireInImportedCJS }; + module.exports = { registerModule, initializeESM, @@ -339,4 +350,5 @@ module.exports = { getConditionsSet, shouldSpawnLoaderHookWorker, compileSourceTextModule, + requestTypes, }; diff --git a/lib/internal/test_runner/mock/mock.js b/lib/internal/test_runner/mock/mock.js index fac97e51b6..c018d1690f 100644 --- a/lib/internal/test_runner/mock/mock.js +++ b/lib/internal/test_runner/mock/mock.js @@ -646,9 +646,8 @@ class MockTracker { // If the caller is already a file URL, use it as is. Otherwise, convert it. const hasFileProtocol = StringPrototypeStartsWith(filename, 'file://'); const caller = hasFileProtocol ? filename : pathToFileURL(filename).href; - const { format, url } = sharedState.moduleLoader.resolveSync( - mockSpecifier, caller, kEmptyObject, - ); + const request = { __proto__: null, specifier: mockSpecifier, attributes: kEmptyObject }; + const { format, url } = sharedState.moduleLoader.resolveSync(caller, request); debug('module mock, url = "%s", format = "%s", caller = "%s"', url, format, caller); if (format) { // Format is not yet known for ambiguous files when detection is enabled. validateOneOf(format, 'format', kSupportedFormats); diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs index 9e25232146..f2726c8c15 100644 --- a/test/es-module/test-esm-loader-hooks.mjs +++ b/test/es-module/test-esm-loader-hooks.mjs @@ -119,7 +119,7 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => { assert.strictEqual(signal, null); }); - it('import.meta.resolve of a never-settling resolve', async () => { + it('import.meta.resolve of a never-settling resolve should throw', async () => { const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [ '--no-warnings', '--experimental-loader', @@ -129,7 +129,7 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => { assert.strictEqual(stderr, ''); assert.match(stdout, /^should be output\r?\n$/); - assert.strictEqual(code, 13); + assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); }); @@ -668,13 +668,17 @@ describe('Loader hooks', { concurrency: !process.env.TEST_PARALLEL }, () => { '--eval', ` import {register} from 'node:module'; - register('data:text/javascript,export function initialize(){return new Promise(()=>{})}'); + try { + register('data:text/javascript,export function initialize(){return new Promise(()=>{})}'); + } catch (e) { + console.log('caught', e.code); + } `, ]); assert.strictEqual(stderr, ''); - assert.strictEqual(stdout, ''); - assert.strictEqual(code, 13); + assert.strictEqual(stdout.trim(), 'caught ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED'); + assert.strictEqual(code, 0); assert.strictEqual(signal, null); }); diff --git a/test/es-module/test-esm-loader-modulemap.js b/test/es-module/test-esm-loader-modulemap.js index f581c7f507..b582f4b2aa 100644 --- a/test/es-module/test-esm-loader-modulemap.js +++ b/test/es-module/test-esm-loader-modulemap.js @@ -17,12 +17,9 @@ const stubJsModule = createDynamicModule([], ['default'], jsModuleDataUrl); const stubJsonModule = createDynamicModule([], ['default'], jsonModuleDataUrl); const loader = createModuleLoader(); -const jsModuleJob = new ModuleJob(loader, stubJsModule.module, undefined, - () => new Promise(() => {})); -const jsonModuleJob = new ModuleJob(loader, stubJsonModule.module, - { type: 'json' }, - () => new Promise(() => {})); - +const jsModuleJob = new ModuleJob(loader, jsModuleDataUrl, {}, stubJsModule.module); +const jsonModuleJob = new ModuleJob(loader, jsonModuleDataUrl, + { type: 'json' }, stubJsonModule.module); // LoadCache.set and LoadCache.get store and retrieve module jobs for a // specified url/type tuple; LoadCache.has correctly reports whether such jobs diff --git a/test/fixtures/es-module-loaders/never-settling-resolve-step/import.meta.never-resolve.mjs b/test/fixtures/es-module-loaders/never-settling-resolve-step/import.meta.never-resolve.mjs index fc3a077abe..2bc389cc3e 100644 --- a/test/fixtures/es-module-loaders/never-settling-resolve-step/import.meta.never-resolve.mjs +++ b/test/fixtures/es-module-loaders/never-settling-resolve-step/import.meta.never-resolve.mjs @@ -1,5 +1,8 @@ +import assert from 'node:assert'; console.log('should be output'); -import.meta.resolve('never-settle-resolve'); - -console.log('should not be output'); +assert.throws(() => { + import.meta.resolve('never-settle-resolve'); +}, { + code: 'ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED' +}); diff --git a/test/fixtures/module-hooks/logger-async-hooks.mjs b/test/fixtures/module-hooks/logger-async-hooks.mjs new file mode 100644 index 0000000000..f26d0ce2ae --- /dev/null +++ b/test/fixtures/module-hooks/logger-async-hooks.mjs @@ -0,0 +1,19 @@ +import fs from 'node:fs'; + +export async function resolve(specifier, context, nextResolve) { + fs.writeSync(1, `resolve ${specifier} from ${context.parentURL}\n`); + const result = await nextResolve(specifier, context); + result.shortCircuit = true; + return result; +} + +export async function load(url, context, nextLoad) { + fs.writeSync(1, `load ${url}\n`); + const result = await nextLoad(url, context); + result.shortCircuit = true; + // Handle the async loader hook quirk where source can be nullish for CommonJS. + // If this returns that nullish value verbatim, the `require` in imported + // CJS won't get the hook invoked. + result.source ??= fs.readFileSync(new URL(url), 'utf8'); + return result; +} diff --git a/test/fixtures/module-hooks/register-logger-async-hooks.mjs b/test/fixtures/module-hooks/register-logger-async-hooks.mjs new file mode 100644 index 0000000000..28a6ae5f22 --- /dev/null +++ b/test/fixtures/module-hooks/register-logger-async-hooks.mjs @@ -0,0 +1,2 @@ +import { register } from 'node:module'; +register('./logger-async-hooks.mjs', import.meta.url); diff --git a/test/fixtures/module-hooks/require-esm/cjs.cjs b/test/fixtures/module-hooks/require-esm/cjs.cjs new file mode 100644 index 0000000000..c4a229c0cc --- /dev/null +++ b/test/fixtures/module-hooks/require-esm/cjs.cjs @@ -0,0 +1 @@ +exports.cjsValue = require('./inner.cjs'); \ No newline at end of file diff --git a/test/fixtures/module-hooks/require-esm/esm.mjs b/test/fixtures/module-hooks/require-esm/esm.mjs new file mode 100644 index 0000000000..832a099e38 --- /dev/null +++ b/test/fixtures/module-hooks/require-esm/esm.mjs @@ -0,0 +1,2 @@ +export { esmValue } from './inner.mjs' +export { cjsValue } from './cjs.cjs' \ No newline at end of file diff --git a/test/fixtures/module-hooks/require-esm/inner.cjs b/test/fixtures/module-hooks/require-esm/inner.cjs new file mode 100644 index 0000000000..89de7dbd40 --- /dev/null +++ b/test/fixtures/module-hooks/require-esm/inner.cjs @@ -0,0 +1 @@ +module.exports = 'commonjs'; diff --git a/test/fixtures/module-hooks/require-esm/inner.mjs b/test/fixtures/module-hooks/require-esm/inner.mjs new file mode 100644 index 0000000000..ed57030dff --- /dev/null +++ b/test/fixtures/module-hooks/require-esm/inner.mjs @@ -0,0 +1 @@ +export const esmValue = 'esm'; diff --git a/test/fixtures/module-hooks/require-esm/main.cjs b/test/fixtures/module-hooks/require-esm/main.cjs new file mode 100644 index 0000000000..d7997a1735 --- /dev/null +++ b/test/fixtures/module-hooks/require-esm/main.cjs @@ -0,0 +1,4 @@ +const { esmValue, cjsValue } = require('./esm.mjs'); +console.log('esmValue in main.cjs:', esmValue); +console.log('cjsValue in main.cjs:', cjsValue); +module.exports = { esmValue, cjsValue }; diff --git a/test/fixtures/module-hooks/require-esm/main.mjs b/test/fixtures/module-hooks/require-esm/main.mjs new file mode 100644 index 0000000000..c875685e0d --- /dev/null +++ b/test/fixtures/module-hooks/require-esm/main.mjs @@ -0,0 +1,3 @@ +import { esmValue, cjsValue } from './main.cjs'; +console.log('esmValue in main.mjs:', esmValue); +console.log('cjsValue in main.mjs:', cjsValue); diff --git a/test/module-hooks/test-module-hooks-require-esm.js b/test/module-hooks/test-module-hooks-require-esm.js new file mode 100644 index 0000000000..a3bb0dd2c4 --- /dev/null +++ b/test/module-hooks/test-module-hooks-require-esm.js @@ -0,0 +1,72 @@ +'use strict'; + +// This tests that the async loader hooks can be invoked for require(esm). + +require('../common'); +const common = require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); +const assert = require('assert'); + +function assertSubGraph(output) { + // FIXME(node:59666): the async resolve hook invoked for require() in imported CJS only get the URL, + // not the specifier. This may be fixable if we re-implement the async loader hooks from within + // the synchronous loader hooks and use the original require() implementation. + assert.match(output, /resolve .+esm\.mjs from file:.+main\.cjs/); + assert.match(output, /load file:.+esm\.mjs/); + + assert.match(output, /resolve \.\/inner\.mjs from file:.+esm\.mjs/); + assert.match(output, /load file:.+inner\.mjs/); + + assert.match(output, /resolve \.\/cjs\.cjs from file:.+esm\.mjs/); + assert.match(output, /load file:.+cjs\.cjs/); + + // FIXME(node:59666): see above. + assert.match(output, /resolve .+inner\.cjs from file:.+cjs\.cjs/); + assert.match(output, /load file:.+inner\.cjs/); + + assert.match(output, /esmValue in main\.cjs: esm/); + assert.match(output, /cjsValue in main\.cjs: commonjs/); +} + +spawnSyncAndAssert(process.execPath, [ + '--import', + fixtures.fileURL('module-hooks', 'register-logger-async-hooks.mjs'), + fixtures.path('module-hooks', 'require-esm', 'main.cjs'), +], { + stdout: common.mustCall((output) => { + // The graph is: + // main.cjs + // -> esm.mjs + // -> inner.mjs + // -> cjs.cjs + // -> inner.cjs + assert.match(output, /resolve .+main\.cjs from undefined/); + assert.match(output, /load file:.+main\.cjs/); + + assertSubGraph(output); + }), +}); + +spawnSyncAndAssert(process.execPath, [ + '--import', + fixtures.fileURL('module-hooks', 'register-logger-async-hooks.mjs'), + fixtures.path('module-hooks', 'require-esm', 'main.mjs'), +], { + stdout: common.mustCall((output) => { + // The graph is: + // main.mjs + // -> main.cjs + // -> esm.mjs + // -> inner.mjs + // -> cjs.cjs + // -> inner.cjs + assert.match(output, /resolve .+main\.mjs from undefined/); + assert.match(output, /load file:.+main\.mjs/); + + assert.match(output, /resolve .+main\.cjs from file:.+main\.mjs/); + assert.match(output, /load file:.+main\.cjs/); + + assertSubGraph(output); + }), +});