From 12f6da731c385177abfad2bc2a17579956b2d35a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ma=C3=ABl=20Nison?= <nison.mael@gmail.com>
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 <jasnell@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
---
 doc/api/esm.md                                |  5 +-
 lib/internal/modules/esm/loader.js            |  4 +-
 lib/internal/process/esm_loader.js            | 49 +++++++++++++------
 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(+), 24 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 9a422e8f70f1ce..88d3caab72c167 100644
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@ -310,7 +310,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()).
@@ -357,8 +357,6 @@ class ESMLoader {
         );
       }
     }
-
-    this.preload();
   }
 
   async eval(
diff --git a/lib/internal/process/esm_loader.js b/lib/internal/process/esm_loader.js
index 25a2b7191cb5d4..d040d8e068ecfa 100644
--- a/lib/internal/process/esm_loader.js
+++ b/lib/internal/process/esm_loader.js
@@ -2,7 +2,7 @@
 
 const {
   ArrayIsArray,
-  ObjectCreate,
+  ArrayPrototypePushApply,
 } = primordials;
 
 const { ESMLoader } = require('internal/modules/esm/loader');
@@ -10,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;
@@ -29,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,
-    ObjectCreate(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<HookName>:', 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\//, `./`));
+}