From 7aac21e90a34260646980a6bc1a5515a9ebdfc4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ma=C3=ABl=20Nison?= Date: Sat, 17 Dec 2022 05:23:11 +0100 Subject: [PATCH] esm: leverage loaders when resolving subsequent loaders PR-URL: https://github.com/nodejs/node/pull/43772 Reviewed-By: James M Snell Reviewed-By: Jacob Smith Reviewed-By: Geoffrey Booth --- doc/api/esm.md | 5 +- lib/internal/modules/esm/loader.js | 4 +- lib/internal/process/esm_loader.js | 48 ++++++++++++++----- test/es-module/test-esm-loader-chaining.mjs | 18 ++++++- .../loader-load-foo-or-42.mjs | 10 +++- .../loader-load-incomplete.mjs | 10 +++- .../loader-load-passthru.mjs | 8 ++++ .../es-module-loaders/loader-resolve-42.mjs | 8 ++++ .../es-module-loaders/loader-resolve-foo.mjs | 8 ++++ .../loader-resolve-incomplete.mjs | 10 +++- .../loader-resolve-next-modified.mjs | 8 ++++ .../loader-resolve-passthru.mjs | 8 ++++ .../loader-resolve-shortcircuit.mjs | 10 +++- .../loader-resolve-strip-xxx.mjs | 4 ++ .../loader-resolve-strip-yyy.mjs | 4 ++ 15 files changed, 140 insertions(+), 23 deletions(-) create mode 100644 test/fixtures/es-module-loaders/loader-resolve-strip-xxx.mjs create mode 100644 test/fixtures/es-module-loaders/loader-resolve-strip-yyy.mjs diff --git a/doc/api/esm.md b/doc/api/esm.md index d67cb47abe526e..9a4273940a7ce0 100644 --- a/doc/api/esm.md +++ b/doc/api/esm.md @@ -701,8 +701,9 @@ changes: To customize the default module resolution, loader hooks can optionally be provided via a `--experimental-loader ./loader-name.mjs` argument to Node.js. -When hooks are used they apply to the entry point and all `import` calls. They -won't apply to `require` calls; those still follow [CommonJS][] rules. +When hooks are used they apply to each subsequent loader, the entry point, and +all `import` calls. They won't apply to `require` calls; those still follow +[CommonJS][] rules. Loaders follow the pattern of `--require`: diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 38cd2777dab9c5..a02619818ec78b 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -309,7 +309,7 @@ class ESMLoader { /** * Collect custom/user-defined hook(s). After all hooks have been collected, - * calls global preload hook(s). + * the global preload hook(s) must be called. * @param {KeyedExports} customLoaders * A list of exports from user-defined loaders (as returned by * ESMLoader.import()). @@ -356,8 +356,6 @@ class ESMLoader { ); } } - - this.preload(); } async eval( diff --git a/lib/internal/process/esm_loader.js b/lib/internal/process/esm_loader.js index 975cc1f1cca4f5..d040d8e068ecfa 100644 --- a/lib/internal/process/esm_loader.js +++ b/lib/internal/process/esm_loader.js @@ -2,6 +2,7 @@ const { ArrayIsArray, + ArrayPrototypePushApply, } = primordials; const { ESMLoader } = require('internal/modules/esm/loader'); @@ -9,6 +10,7 @@ const { hasUncaughtExceptionCaptureCallback, } = require('internal/process/execution'); const { pathToFileURL } = require('internal/url'); +const { kEmptyObject } = require('internal/util'); const esmLoader = new ESMLoader(); exports.esmLoader = esmLoader; @@ -28,41 +30,61 @@ async function initializeLoader() { const { getOptionValue } = require('internal/options'); const customLoaders = getOptionValue('--experimental-loader'); const preloadModules = getOptionValue('--import'); - const loaders = await loadModulesInIsolation(customLoaders); + + let cwd; + try { + cwd = process.cwd() + '/'; + } catch { + cwd = '/'; + } + + const internalEsmLoader = new ESMLoader(); + const allLoaders = []; + + const parentURL = pathToFileURL(cwd).href; + + for (let i = 0; i < customLoaders.length; i++) { + const customLoader = customLoaders[i]; + + // Importation must be handled by internal loader to avoid polluting user-land + const keyedExportsSublist = await internalEsmLoader.import( + [customLoader], + parentURL, + kEmptyObject, + ); + + internalEsmLoader.addCustomLoaders(keyedExportsSublist); + ArrayPrototypePushApply(allLoaders, keyedExportsSublist); + } // Hooks must then be added to external/public loader // (so they're triggered in userland) - esmLoader.addCustomLoaders(loaders); + esmLoader.addCustomLoaders(allLoaders); + esmLoader.preload(); // Preload after loaders are added so they can be used if (preloadModules?.length) { - await loadModulesInIsolation(preloadModules, loaders); + await loadModulesInIsolation(parentURL, preloadModules, allLoaders); } isESMInitialized = true; } -function loadModulesInIsolation(specifiers, loaders = []) { +function loadModulesInIsolation(parentURL, specifiers, loaders = []) { if (!ArrayIsArray(specifiers) || specifiers.length === 0) { return; } - let cwd; - try { - cwd = process.cwd() + '/'; - } catch { - cwd = 'file:///'; - } - // A separate loader instance is necessary to avoid cross-contamination // between internal Node.js and userland. For example, a module with internal // state (such as a counter) should be independent. const internalEsmLoader = new ESMLoader(); internalEsmLoader.addCustomLoaders(loaders); + internalEsmLoader.preload(); // Importation must be handled by internal loader to avoid poluting userland return internalEsmLoader.import( specifiers, - pathToFileURL(cwd).href, - { __proto__: null }, + parentURL, + kEmptyObject, ); } diff --git a/test/es-module/test-esm-loader-chaining.mjs b/test/es-module/test-esm-loader-chaining.mjs index b04dbe4ddd6c1a..a42020ef42f8dd 100644 --- a/test/es-module/test-esm-loader-chaining.mjs +++ b/test/es-module/test-esm-loader-chaining.mjs @@ -4,7 +4,6 @@ import assert from 'node:assert'; import { execPath } from 'node:process'; import { describe, it } from 'node:test'; - const setupArgs = [ '--no-warnings', '--input-type=module', @@ -253,6 +252,23 @@ describe('ESM: loader chaining', { concurrency: true }, () => { assert.strictEqual(code, 0); }); + it('should allow loaders to influence subsequent loader resolutions', async () => { + const { code, stderr } = await spawnPromisified( + execPath, + [ + '--loader', + fixtures.fileURL('es-module-loaders', 'loader-resolve-strip-xxx.mjs'), + '--loader', + 'xxx/loader-resolve-strip-yyy.mjs', + ...commonArgs, + ], + { encoding: 'utf8', cwd: fixtures.path('es-module-loaders') }, + ); + + assert.strictEqual(stderr, ''); + assert.strictEqual(code, 0); + }); + it('should throw when the resolve chain is broken', async () => { const { code, stderr, stdout } = await spawnPromisified( execPath, diff --git a/test/fixtures/es-module-loaders/loader-load-foo-or-42.mjs b/test/fixtures/es-module-loaders/loader-load-foo-or-42.mjs index 8d408223e66a0a..8f850e82bef54b 100644 --- a/test/fixtures/es-module-loaders/loader-load-foo-or-42.mjs +++ b/test/fixtures/es-module-loaders/loader-load-foo-or-42.mjs @@ -1,4 +1,12 @@ -export async function load(url) { +export async function load(url, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (url.includes('loader')) { + return next(url); + } + const val = url.includes('42') ? '42' : '"foo"'; diff --git a/test/fixtures/es-module-loaders/loader-load-incomplete.mjs b/test/fixtures/es-module-loaders/loader-load-incomplete.mjs index d6242488e5738e..a4bf3531f3d225 100644 --- a/test/fixtures/es-module-loaders/loader-load-incomplete.mjs +++ b/test/fixtures/es-module-loaders/loader-load-incomplete.mjs @@ -1,4 +1,12 @@ -export async function load() { +export async function load(url, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (url.includes('loader')) { + return next(url); + } + return { format: 'module', source: 'export default 42', diff --git a/test/fixtures/es-module-loaders/loader-load-passthru.mjs b/test/fixtures/es-module-loaders/loader-load-passthru.mjs index 0de06142007562..1c2f2ea4487cdf 100644 --- a/test/fixtures/es-module-loaders/loader-load-passthru.mjs +++ b/test/fixtures/es-module-loaders/loader-load-passthru.mjs @@ -1,4 +1,12 @@ export async function load(url, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (url.includes('loader')) { + return next(url); + } + console.log('load passthru'); // This log is deliberate return next(url); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-42.mjs b/test/fixtures/es-module-loaders/loader-resolve-42.mjs index eaca111998ae23..05ef2f68390bd1 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-42.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-42.mjs @@ -1,4 +1,12 @@ export async function resolve(specifier, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (specifier.includes('loader')) { + return next(specifier); + } + console.log('resolve 42'); // This log is deliberate console.log('next:', next.name); // This log is deliberate diff --git a/test/fixtures/es-module-loaders/loader-resolve-foo.mjs b/test/fixtures/es-module-loaders/loader-resolve-foo.mjs index 7d23d6c49088c9..09d30e952a2a92 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-foo.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-foo.mjs @@ -1,4 +1,12 @@ export async function resolve(specifier, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (specifier.includes('loader')) { + return next(specifier); + } + console.log('resolve foo'); // This log is deliberate return next('file:///foo.mjs'); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs b/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs index 9eb1617f30130e..d00b8fc50134e1 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-incomplete.mjs @@ -1,4 +1,12 @@ -export async function resolve() { +export async function resolve(specifier, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (specifier.includes('loader')) { + return next(specifier); + } + return { url: 'file:///incomplete-resolve-chain.js', }; diff --git a/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs b/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs index a973345a82ff21..ac5ad5b1fe020d 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-next-modified.mjs @@ -1,4 +1,12 @@ export async function resolve(url, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (url.includes('loader')) { + return next(url); + } + const { format, url: nextUrl, diff --git a/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs b/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs index 3db5b21bb98793..d4845acc9f5f71 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-passthru.mjs @@ -1,4 +1,12 @@ export async function resolve(specifier, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (specifier.includes('loader')) { + return next(specifier); + } + console.log('resolve passthru'); // This log is deliberate return next(specifier); } diff --git a/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs b/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs index d886b3dfcbf237..e1a357b4ab48f9 100644 --- a/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs +++ b/test/fixtures/es-module-loaders/loader-resolve-shortcircuit.mjs @@ -1,4 +1,12 @@ -export async function resolve(specifier) { +export async function resolve(specifier, context, next) { + // This check is needed to make sure that we don't prevent the + // resolution from follow-up loaders. It wouldn't be a problem + // in real life because loaders aren't supposed to break the + // resolution, but the ones used in our tests do, for convenience. + if (specifier.includes('loader')) { + return next(specifier); + } + return { shortCircuit: true, url: specifier, diff --git a/test/fixtures/es-module-loaders/loader-resolve-strip-xxx.mjs b/test/fixtures/es-module-loaders/loader-resolve-strip-xxx.mjs new file mode 100644 index 00000000000000..58d7a1107994fe --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-resolve-strip-xxx.mjs @@ -0,0 +1,4 @@ +export async function resolve(specifier, context, nextResolve) { + console.log(`loader-a`, {specifier}); + return nextResolve(specifier.replace(/^xxx\//, `./`)); +} diff --git a/test/fixtures/es-module-loaders/loader-resolve-strip-yyy.mjs b/test/fixtures/es-module-loaders/loader-resolve-strip-yyy.mjs new file mode 100644 index 00000000000000..3615926143c4c7 --- /dev/null +++ b/test/fixtures/es-module-loaders/loader-resolve-strip-yyy.mjs @@ -0,0 +1,4 @@ +export async function resolve(specifier, context, nextResolve) { + console.log(`loader-b`, {specifier}); + return nextResolve(specifier.replace(/^yyy\//, `./`)); +}