esm: leverage loaders when resolving subsequent loaders

PR-URL: nodejs/node#43772
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>

Author: sercher

graal-nodejs/doc/api/esm.md

@@ -700,8 +700,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`:
 

graal-nodejs/lib/internal/modules/esm/loader.js

@@ -322,7 +322,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()).
@@ -369,8 +369,6 @@ class ESMLoader {
         );
       }
     }
-
-    this.preload();
   }
 
   async eval(

graal-nodejs/lib/internal/process/esm_loader.js

@@ -2,14 +2,15 @@
 
 const {
   ArrayIsArray,
-  ObjectCreate,
+  ArrayPrototypePushApply,
 } = primordials;
 
 const { ESMLoader } = require('internal/modules/esm/loader');
 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,
   );
 }
 

graal-nodejs/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,

graal-nodejs/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"';

graal-nodejs/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',

graal-nodejs/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);
 }

graal-nodejs/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
 

graal-nodejs/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');
 }

graal-nodejs/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',
   };