diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 2e0b0fef8e9d..0f5096d05dae 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -3,7 +3,9 @@ const { ArrayPrototypeJoin, ArrayPrototypeMap, + ArrayPrototypePush, ArrayPrototypeReduce, + ArrayPrototypeSome, FunctionPrototypeCall, JSONStringify, ObjectSetPrototypeOf, @@ -123,6 +125,7 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/ * responseURL?: string, * translatorKey: string, * isResolvedBySyncHooks: boolean, + * isLoadedBySyncHooks: boolean, * isSourceLoadedSynchronously: boolean, * }} TranslateContext */ @@ -761,7 +764,7 @@ class ModuleLoader { /** * 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. - * @param {{isResolvedByDefaultResolve: boolean}} out Output object to track whether the default resolve was used + * @param {{isResolvedByDefaultStep: boolean}} out Output object to track whether the default step was used * without polluting the user-visible resolve result. * @param {string|URL} specifier See {@link resolveSync}. * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context @@ -769,10 +772,12 @@ class ModuleLoader { * @returns {{ format: string, url: string }} */ #resolveAndMaybeBlockOnLoaderThread(out, specifier, context) { + // Track that the synchronous hooks reached the default step, whether the + // asynchronous hooks or the default resolver handle it. + out.isResolvedByDefaultStep = true; if (this.#asyncLoaderHooks?.resolveSync) { return this.#asyncLoaderHooks.resolveSync(specifier, context.parentURL, context.importAttributes); } - out.isResolvedByDefaultResolve = true; return this.#cachedDefaultResolve(specifier, context); } @@ -793,7 +798,7 @@ class ModuleLoader { const specifier = `${request.specifier}`; const importAttributes = request.attributes ?? kEmptyObject; // Use an output parameter to track the state and avoid polluting the user-visible resolve results. - const out = { isResolvedByDefaultResolve: false, __proto__: null }; + const out = { isResolvedByDefaultStep: false, __proto__: null }; let result; let isResolvedBySyncHooks = false; @@ -802,7 +807,7 @@ class ModuleLoader { result = resolveWithSyncHooks(specifier, parentURL, importAttributes, this.#defaultConditions, this.#resolveAndMaybeBlockOnLoaderThread.bind(this, out)); // If the default step ran, sync hooks did not short-circuit the resolution. - isResolvedBySyncHooks = !out.isResolvedByDefaultResolve; + isResolvedBySyncHooks = !out.isResolvedByDefaultStep; } else { const context = { ...request, @@ -824,6 +829,7 @@ class ModuleLoader { * format: ModuleFormat, * source: ModuleSource, * responseURL?: string, + * isLoadedBySyncHooks: boolean, * isSourceLoadedSynchronously: boolean, * }} LoadResult * @param {string} url The URL of the module to be loaded. @@ -841,20 +847,31 @@ class ModuleLoader { /** * This is the default load step for module.registerHooks(), which incorporates asynchronous hooks * from module.register() which are run in a blocking fashion for it to be synchronous. - * @param {{isSourceLoadedSynchronously: boolean}} out - * Output object to track whether the source was loaded synchronously without polluting - * the user-visible load result. + * @param {{isSourceLoadedSynchronously: boolean, defaultStepResults?: object[]}} out + * Output object to track whether the source was loaded synchronously, and the results + * of this step when there are synchronous hooks, without polluting the user-visible load result. * @param {string} url See {@link load} * @param {object} context See {@link load} * @returns {{ format: ModuleFormat, source: ModuleSource }} */ #loadAndMaybeBlockOnLoaderThread(out, url, context) { + let result; if (this.#asyncLoaderHooks?.loadSync) { out.isSourceLoadedSynchronously = false; - return this.#asyncLoaderHooks.loadSync(url, context); + result = this.#asyncLoaderHooks.loadSync(url, context); + } else { + out.isSourceLoadedSynchronously = true; + result = defaultLoadSync(url, context); + } + if (out.defaultStepResults !== undefined) { + // Copy the format and source, as the hooks may modify the result in place. + ArrayPrototypePush(out.defaultStepResults, { + __proto__: null, + format: result.format, + source: result.source, + }); } - out.isSourceLoadedSynchronously = true; - return defaultLoadSync(url, context); + return result; } /** @@ -870,10 +887,15 @@ class ModuleLoader { // Use an output parameter to track the state and avoid polluting the user-visible resolve results. const out = { isSourceLoadedSynchronously: true, + defaultStepResults: undefined, __proto__: null, }; let result; + let isLoadedBySyncHooks = false; if (syncLoadHooks.length) { + out.defaultStepResults = []; + let finalFormat; + let finalSource; // Has module.registerHooks() hooks, chain the asynchronous hooks in the default step. // TODO(joyeecheung): construct the ModuleLoadContext in the loaders directly instead // of converting them from plain objects in the hooks. @@ -883,11 +905,23 @@ class ModuleLoader { context.importAttributes, this.#defaultConditions, this.#loadAndMaybeBlockOnLoaderThread.bind(this, out), - validateLoadSloppy, + (url, context, result) => { + // The last result validated is the final one, before its source gets decoded. + ({ format: finalFormat, source: finalSource } = result); + return validateLoadSloppy(url, context, result); + }, + ); + // The sync hooks customized the loading if they provided a source, other than passing + // on the format and source from the default step. A nullish source for CommonJS means + // that the CommonJS loader should load it. + isLoadedBySyncHooks = finalSource != null && !ArrayPrototypeSome( + out.defaultStepResults, + (step) => step.format === finalFormat && step.source === finalSource, ); } else { result = this.#loadAndMaybeBlockOnLoaderThread(out, url, context); } + result.isLoadedBySyncHooks = isLoadedBySyncHooks; result.isSourceLoadedSynchronously = out.isSourceLoadedSynchronously; return result; } diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js index 583b6d1fe91d..575eb1e2a828 100644 --- a/lib/internal/modules/esm/translators.js +++ b/lib/internal/modules/esm/translators.js @@ -351,9 +351,9 @@ translators.set('require-commonjs-typescript', (url, translateContext, parentURL // This goes through Module._load to accommodate monkey-patchers. /** * Loads a CommonJS module through Module._load to accommodate monkey-patchers. - * If the module was resolved by synchronous hooks (i.e. not by the default resolver), - * passes the pre-resolved information and source to Module._load to avoid - * re-resolving and re-loading. + * If the module was resolved by synchronous hooks (i.e. without reaching the default step), + * or its format or source was provided by synchronous hooks, passes the pre-resolved + * information and source to Module._load to avoid re-resolving and re-loading. * @param {import('internal/modules/cjs/loader').Module} module - The module to load. * @param {string} source - The source code of the module. * @param {string} url - The URL of the module. @@ -365,7 +365,12 @@ function loadCJSModuleWithModuleLoad(module, source, url, filename, isMain, tran assert(module === CJSModule._cache[filename]); debug(`loadCJSModuleWithModuleLoad ${url}`); let exports; - if (translateContext.isResolvedBySyncHooks) { + // Otherwise the CommonJS loader would resolve the module again, which may not find it, + // and pick the extension handler by the file name instead of the format from the hooks. + // Leave builtins alone, as Module._load() runs the load hooks again for them and + // expects the builtin format. + if (translateContext.isResolvedBySyncHooks || + (translateContext.isLoadedBySyncHooks && !StringPrototypeStartsWith(url, 'node:'))) { exports = wrapModuleLoad(filename, undefined, isMain, { __proto__: null, resolved: { diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/data.json b/test/fixtures/module-hooks/import-cjs-custom-format/data.json new file mode 100644 index 000000000000..0967ef424bce --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/data.json @@ -0,0 +1 @@ +{} diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs b/test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs new file mode 100644 index 000000000000..6d0da5fe8797 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/format-only.mjs @@ -0,0 +1 @@ +module.exports = { name: "format-only.mjs" }; diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/main.js b/test/fixtures/module-hooks/import-cjs-custom-format/main.js new file mode 100644 index 000000000000..4f61ce38d14b --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/main.js @@ -0,0 +1,70 @@ +'use strict'; +// Imports modules that synchronous load hooks provide as CommonJS, while the +// resolve hook passes on the default resolution when there is one. + +const { registerHooks } = require('module'); +const path = require('path'); +const { pathToFileURL } = require('url'); + +const missingURL = pathToFileURL(path.join(__dirname, 'missing.cjs')).href; +const mutatedURL = pathToFileURL(path.join(__dirname, 'mutated.mjs')).href; +const formatOnlyURL = pathToFileURL(path.join(__dirname, 'format-only.mjs')).href; +const dataURL = 'data:text/javascript,export default "from data URL";'; + +const names = new Map([ + [pathToFileURL(path.join(__dirname, 'data.json')).href, 'data.json'], + [pathToFileURL(path.join(__dirname, 'module.mjs')).href, 'module.mjs'], + [missingURL, 'missing.cjs'], + [dataURL, 'data URL'], +]); + +registerHooks({ + resolve(specifier, context, nextResolve) { + try { + return nextResolve(specifier, context); + } catch (err) { + if (specifier === './missing.cjs') { + return { url: missingURL, shortCircuit: true }; + } + throw err; + } + }, + load(url, context, nextLoad) { + // The two cases below customize the result from the default step. With + // asynchronous hooks, they still use the re-invented require(), so they don't + // report require.cache. + // TODO: load them with the CommonJS loader in that case too. + if (url === mutatedURL) { + // Modify the result in place. + const result = nextLoad(url, context); + result.format = 'commonjs'; + result.source = 'module.exports = { name: "mutated.mjs" };'; + return result; + } + if (url === formatOnlyURL) { + // Only override the format of the CommonJS source in a .mjs file. + return { ...nextLoad(url, context), format: 'commonjs' }; + } + const name = names.get(url); + if (name !== undefined) { + return { + format: 'commonjs', + source: `module.exports = { name: ${JSON.stringify(name)}, ` + + 'hasRequireCache: require.cache !== undefined };', + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, +}); + +// Import the modules one by one to keep the output in order. +let imported = Promise.resolve(); +const specifiers = [ + './data.json', './module.mjs', './missing.cjs', dataURL, './mutated.mjs', './format-only.mjs', +]; +for (const specifier of specifiers) { + imported = imported + .then(() => import(specifier)) + .then(({ default: result }) => console.log(JSON.stringify(result))); +} diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/module.mjs b/test/fixtures/module-hooks/import-cjs-custom-format/module.mjs new file mode 100644 index 000000000000..0b5728904626 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/module.mjs @@ -0,0 +1 @@ +export default 'from disk'; diff --git a/test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs b/test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs new file mode 100644 index 000000000000..0b5728904626 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-custom-format/mutated.mjs @@ -0,0 +1 @@ +export default 'from disk'; diff --git a/test/fixtures/module-hooks/import-cjs-require-extensions/main.js b/test/fixtures/module-hooks/import-cjs-require-extensions/main.js new file mode 100644 index 000000000000..12da05e7c1d1 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-require-extensions/main.js @@ -0,0 +1,45 @@ +'use strict'; +// Imports a .ts file as CommonJS, with a resolve hook that only overrides the +// format, and a load hook that doesn't provide its own source in the way named +// by the first argument. The require.extensions handler for .ts should load it. + +const fs = require('fs'); +const Module = require('module'); +const path = require('path'); +const { pathToFileURL } = require('url'); + +const targetURL = pathToFileURL(path.join(__dirname, 'target.ts')).href; + +Module._extensions['.ts'] = (mod, filename) => { + const source = fs.readFileSync(filename, 'utf8'); + mod._compile(source.replace('original', 'transformed'), filename); +}; + +const loadHooks = { + 'pass-through': (url, context, nextLoad) => nextLoad(url, context), + 'spread': (url, context, nextLoad) => ({ ...nextLoad(url, context) }), + 'next-twice': (url, context, nextLoad) => { + const result = nextLoad(url, context); + nextLoad(url, context); + return result; + }, + 'nullish-source': (url, context, nextLoad) => { + if (url === targetURL) { + return { format: 'commonjs', source: null, shortCircuit: true }; + } + return nextLoad(url, context); + }, +}; + +Module.registerHooks({ + resolve(specifier, context, nextResolve) { + const result = nextResolve(specifier, context); + if (result.url === targetURL) { + return { ...result, format: 'commonjs' }; + } + return result; + }, + load: loadHooks[process.argv[2]], +}); + +import(targetURL).then(({ default: result }) => console.log(result)); diff --git a/test/fixtures/module-hooks/import-cjs-require-extensions/target.ts b/test/fixtures/module-hooks/import-cjs-require-extensions/target.ts new file mode 100644 index 000000000000..b0a7d69e8b40 --- /dev/null +++ b/test/fixtures/module-hooks/import-cjs-require-extensions/target.ts @@ -0,0 +1 @@ +module.exports = 'original'; diff --git a/test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js b/test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js new file mode 100644 index 000000000000..d3cd55eb2e0c --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-builtin-override-commonjs-import.js @@ -0,0 +1,30 @@ +'use strict'; + +// This tests that load hooks can override the format of imported builtin +// modules to 'commonjs' after calling nextLoad(). +const common = require('../common'); +const assert = require('assert'); +const { registerHooks } = require('module'); + +// Pick a builtin that's unlikely to be loaded already - like zlib. +assert(!process.moduleLoadList.includes('NativeModule zlib')); + +const hook = registerHooks({ + load: common.mustCall(function load(url, context, nextLoad) { + const result = nextLoad(url, context); + if (url === 'node:zlib' && result.format === 'builtin') { + return { + source: 'exports.custom_zlib = "overridden by load hook";', + format: 'commonjs', + shortCircuit: true, + }; + } + return result; + }, 2), // Called by the ESM loader, then again by Module._load() for the builtin. +}); + +import('node:zlib').then(common.mustCall((ns) => { + assert.strictEqual(ns.default.custom_zlib, 'overridden by load hook'); + assert.strictEqual(ns.default.createGzip, undefined); + hook.deregister(); +})); diff --git a/test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js b/test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js new file mode 100644 index 000000000000..7694db228cbc --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-import-cjs-custom-format.js @@ -0,0 +1,30 @@ +'use strict'; +// Test that imported CJS uses the format and source from synchronous load hooks, +// with and without asynchronous hooks from module.register(), when the resolve +// hooks don't short-circuit, or the module is not on disk. When the load hooks +// don't call nextLoad(), the module also gets the full CommonJS require(). +// Refs: https://github.com/nodejs/node/issues/63060 +// Refs: https://github.com/nodejs/node/issues/66998 + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const main = fixtures.path('module-hooks', 'import-cjs-custom-format', 'main.js'); +const asyncForward = fixtures.fileURL('module-hooks', 'sync-and-async', 'async-forward.js').href; + +const stdout = [ + '{"name":"data.json","hasRequireCache":true}', + '{"name":"module.mjs","hasRequireCache":true}', + '{"name":"missing.cjs","hasRequireCache":true}', + '{"name":"data URL","hasRequireCache":true}', + '{"name":"mutated.mjs"}', + '{"name":"format-only.mjs"}', +].join('\n'); + +for (const preload of [[], ['--import', asyncForward]]) { + spawnSyncAndAssert(process.execPath, [...preload, main], { + stdout, + trim: true, + }); +} diff --git a/test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js b/test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js new file mode 100644 index 000000000000..58872f5a6371 --- /dev/null +++ b/test/module-hooks/test-module-hooks-load-import-cjs-custom-source-async-resolve.js @@ -0,0 +1,55 @@ +'use strict'; +// Test that imported CJS resolved to a non-file URL, or to a file that is not on +// disk, by asynchronous hooks from module.register(), with the source provided by +// synchronous hooks from module.registerHooks(), won't be resolved again by the +// CJS loader (in which case it would throw because the module is not on disk). + +const common = require('../common'); +const assert = require('assert'); +const path = require('path'); +const { register, registerHooks } = require('module'); +const { pathToFileURL } = require('url'); + +const missingURL = pathToFileURL(path.join(__dirname, 'missing.cjs')).href; +const sources = new Map([ + ['virtual:entry', 'module.exports = "from virtual URL";'], + [missingURL, 'module.exports = "from missing file";'], +]); + +const asyncHooks = ` +export async function resolve(specifier, context, nextResolve) { + if (specifier === 'virtual:entry') { + return { url: specifier, format: 'commonjs', shortCircuit: true }; + } + if (specifier === 'missing') { + return { url: ${JSON.stringify(missingURL)}, format: 'commonjs', shortCircuit: true }; + } + return nextResolve(specifier, context); +} +`; +register(`data:text/javascript,${encodeURIComponent(asyncHooks)}`); + +const hook = registerHooks({ + resolve: common.mustCall((specifier, context, nextResolve) => { + return nextResolve(specifier, context); + }, 2), + load: common.mustCall((url, context, nextLoad) => { + if (sources.has(url)) { + return { + format: 'commonjs', + source: sources.get(url), + shortCircuit: true, + }; + } + return nextLoad(url, context); + }, 2), +}); + +Promise.all([ + import('virtual:entry'), + import('missing'), +]).then(common.mustCall(([virtual, missing]) => { + assert.strictEqual(virtual.default, 'from virtual URL'); + assert.strictEqual(missing.default, 'from missing file'); + hook.deregister(); +})); diff --git a/test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js b/test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js new file mode 100644 index 000000000000..0a83416c2f54 --- /dev/null +++ b/test/module-hooks/test-module-hooks-resolve-import-cjs-require-extensions.js @@ -0,0 +1,24 @@ +'use strict'; +// Test that when module.registerHooks() resolve hooks call nextResolve() and only +// override the format to 'commonjs', and the load hooks don't provide their own +// source, the imported CJS module is still loaded through require.extensions, +// with or without asynchronous hooks from module.register(). The asynchronous +// hooks run in the default step of the synchronous hooks, so passing through +// them should not count as a short-circuit. +// Refs: https://github.com/nodejs/node/issues/66998 + +require('../common'); +const { spawnSyncAndAssert } = require('../common/child_process'); +const fixtures = require('../common/fixtures'); + +const main = fixtures.path('module-hooks', 'import-cjs-require-extensions', 'main.js'); +const asyncForward = fixtures.fileURL('module-hooks', 'sync-and-async', 'async-forward.js').href; + +for (const preload of [[], ['--import', asyncForward]]) { + for (const load of ['pass-through', 'spread', 'next-twice', 'nullish-source']) { + spawnSyncAndAssert(process.execPath, [...preload, main, load], { + stdout: 'transformed', + trim: true, + }); + } +}