module: fix conditions override in synchronous resolve hooks

1. Make sure that the conditions are converted into arrays when
  being passed into user hooks.
2. Pass the conditions from user hooks into the ESM resolution
  so that it takes effect.

PR-URL: #59011
Fixes: #59003
Reviewed-By: Zeyu "Alex" Yang <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -51,6 +51,7 @@ const {
   ReflectSet,
   RegExpPrototypeExec,
   SafeMap,
+  SafeSet,
   String,
   StringPrototypeCharAt,
   StringPrototypeCharCodeAt,
@@ -154,6 +155,7 @@ const internalFsBinding = internalBinding('fs');
 const { safeGetenv } = internalBinding('credentials');
 const {
   getCjsConditions,
+  getCjsConditionsArray,
   initializeCjsConditions,
   loadBuiltinModule,
   makeRequireFunction,
@@ -653,7 +655,7 @@ const EXPORTS_PATTERN = /^((?:@[^/\\%]+\/)?[^./\\%][^/\\%]*)(\/.*)?$/;
  * Resolves the exports for a given module path and request.
  * @param {string} nmPath The path to the module.
  * @param {string} request The request for the module.
- * @param {unknown} conditions
+ * @param {Set<string>} conditions The conditions to use for resolution.
  * @returns {undefined|string}
  */
 function resolveExports(nmPath, request, conditions) {
@@ -1068,17 +1070,30 @@ function resolveForCJSWithHooks(specifier, parent, isMain) {
   function defaultResolve(specifier, context) {
     // TODO(joyeecheung): parent and isMain should be part of context, then we
     // no longer need to use a different defaultResolve for every resolution.
+    // In the hooks, context.conditions is passed around as an array, but internally
+    // the resolution helpers expect a SafeSet. Do the conversion here.
+    let conditionSet;
+    const conditions = context.conditions;
+    if (conditions !== undefined && conditions !== getCjsConditionsArray()) {
+      if (!ArrayIsArray(conditions)) {
+        throw new ERR_INVALID_ARG_VALUE('context.conditions', conditions,
+                                        'expected an array');
+      }
+      conditionSet = new SafeSet(conditions);
+    } else {
+      conditionSet = getCjsConditions();
+    }
     defaultResolvedFilename = defaultResolveImpl(specifier, parent, isMain, {
       __proto__: null,
-      conditions: context.conditions,
+      conditions: conditionSet,
     });
 
     defaultResolvedURL = convertCJSFilenameToURL(defaultResolvedFilename);
     return { __proto__: null, url: defaultResolvedURL };
   }
 
   const resolveResult = resolveWithHooks(specifier, parentURL, /* importAttributes */ undefined,
-                                         getCjsConditions(), defaultResolve);
+                                         getCjsConditionsArray(), defaultResolve);
   const { url } = resolveResult;
   format = resolveResult.format;
 
@@ -1154,7 +1169,7 @@ function loadBuiltinWithHooks(id, url, format) {
     url ??= `node:${id}`;
     // TODO(joyeecheung): do we really want to invoke the load hook for the builtins?
     const loadResult = loadWithHooks(url, format || 'builtin', /* importAttributes */ undefined,
-                                     getCjsConditions(), getDefaultLoad(url, id));
+                                     getCjsConditionsArray(), getDefaultLoad(url, id));
     if (loadResult.format && loadResult.format !== 'builtin') {
       return undefined;  // Format has been overridden, return undefined for the caller to continue loading.
     }
@@ -1306,7 +1321,7 @@ Module._load = function(request, parent, isMain) {
  * @param {ResolveFilenameOptions} options Options object
  * @typedef {object} ResolveFilenameOptions
  * @property {string[]} paths Paths to search for modules in
- * @property {string[]} conditions Conditions used for resolution.
+ * @property {Set<string>?} conditions The conditions to use for resolution.
  * @returns {void|string}
  */
 Module._resolveFilename = function(request, parent, isMain, options) {
@@ -1755,7 +1770,8 @@ function loadSource(mod, filename, formatFromNode) {
     mod[kURL] = convertCJSFilenameToURL(filename);
   }
 
-  const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined, getCjsConditions(),
+  const loadResult = loadWithHooks(mod[kURL], mod[kFormat], /* importAttributes */ undefined,
+                                   getCjsConditionsArray(),
                                    getDefaultLoad(mod[kURL], filename));
 
   // Reset the module properties with load hook results.

lib/internal/modules/esm/loader.js

@@ -709,24 +709,30 @@ class ModuleLoader {
     if (this.#customizations) {  // Only has module.register hooks.
       return this.#customizations.resolve(specifier, parentURL, importAttributes);
     }
-    return this.#cachedDefaultResolve(specifier, parentURL, importAttributes);
+    return this.#cachedDefaultResolve(specifier, {
+      __proto__: null,
+      conditions: this.#defaultConditions,
+      parentURL,
+      importAttributes,
+    });
   }
 
   /**
    * Either return a cached resolution, or perform the default resolution which is synchronous, and
    * cache the result.
    * @param {string} specifier See {@link resolve}.
-   * @param {string} [parentURL] See {@link resolve}.
-   * @param {ImportAttributes} importAttributes See {@link resolve}.
+   * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
    * @returns {{ format: string, url: string }}
    */
-  #cachedDefaultResolve(specifier, parentURL, importAttributes) {
+  #cachedDefaultResolve(specifier, context) {
+    const { parentURL, importAttributes } = context;
     const requestKey = this.#resolveCache.serializeKey(specifier, importAttributes);
     const cachedResult = this.#resolveCache.get(requestKey, parentURL);
     if (cachedResult != null) {
       return cachedResult;
     }
-    const result = this.defaultResolve(specifier, parentURL, importAttributes);
+    defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
+    const result = defaultResolve(specifier, context);
     this.#resolveCache.set(requestKey, parentURL, result);
     return result;
   }
@@ -754,14 +760,15 @@ 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 {string|URL} specifier See {@link resolveSync}.
-   * @param {{ parentURL?: string, importAttributes: ImportAttributes}} context See {@link resolveSync}.
+   * @param {{ parentURL?: string, importAttributes: ImportAttributes, conditions?: string[]}} context
+   *   See {@link resolveSync}.
    * @returns {{ format: string, url: string }}
    */
   #resolveAndMaybeBlockOnLoaderThread(specifier, context) {
     if (this.#customizations) {
       return this.#customizations.resolveSync(specifier, context.parentURL, context.importAttributes);
     }
-    return this.#cachedDefaultResolve(specifier, context.parentURL, context.importAttributes);
+    return this.#cachedDefaultResolve(specifier, context);
   }
 
   /**
@@ -784,26 +791,12 @@ class ModuleLoader {
       return resolveWithHooks(specifier, parentURL, importAttributes, this.#defaultConditions,
                               this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
     }
-    return this.#resolveAndMaybeBlockOnLoaderThread(specifier, { parentURL, importAttributes });
-  }
-
-  /**
-   * Our `defaultResolve` is synchronous and can be used in both
-   * `resolve` and `resolveSync`. This function is here just to avoid
-   * repeating the same code block twice in those functions.
-   * @returns {string}
-   */
-  defaultResolve(originalSpecifier, parentURL, importAttributes) {
-    defaultResolve ??= require('internal/modules/esm/resolve').defaultResolve;
-
-    const context = {
+    return this.#resolveAndMaybeBlockOnLoaderThread(specifier, {
       __proto__: null,
       conditions: this.#defaultConditions,
-      importAttributes,
       parentURL,
-    };
-
-    return defaultResolve(originalSpecifier, context);
+      importAttributes,
+    });
   }
 
   /**

lib/internal/modules/helpers.js

@@ -66,6 +66,9 @@ function toRealPath(requestPath) {
 
 /** @type {Set<string>} */
 let cjsConditions;
+/** @type {string[]} */
+let cjsConditionsArray;
+
 /**
  * Define the conditions that apply to the CommonJS loader.
  * @returns {void}
@@ -75,15 +78,17 @@ function initializeCjsConditions() {
   const noAddons = getOptionValue('--no-addons');
   const addonConditions = noAddons ? [] : ['node-addons'];
   // TODO: Use this set when resolving pkg#exports conditions in loader.js.
-  cjsConditions = new SafeSet([
+  cjsConditionsArray = [
     'require',
     'node',
     ...addonConditions,
     ...userConditions,
-  ]);
+  ];
   if (getOptionValue('--experimental-require-module')) {
-    cjsConditions.add('module-sync');
+    cjsConditionsArray.push('module-sync');
   }
+  ObjectFreeze(cjsConditionsArray);
+  cjsConditions = new SafeSet(cjsConditionsArray);
 }
 
 /**
@@ -97,6 +102,13 @@ function getCjsConditions() {
   return cjsConditions;
 }
 
+function getCjsConditionsArray() {
+  if (cjsConditionsArray === undefined) {
+    initializeCjsConditions();
+  }
+  return cjsConditionsArray;
+}
+
 /**
  * Provide one of Node.js' public modules to user code.
  * @param {string} id - The identifier/specifier of the builtin module to load
@@ -418,6 +430,7 @@ module.exports = {
   flushCompileCache,
   getBuiltinModule,
   getCjsConditions,
+  getCjsConditionsArray,
   getCompileCacheDir,
   initializeCjsConditions,
   loadBuiltinModule,

test/fixtures/es-modules/custom-condition/node_modules/foo/default.cjs

@@ -0,0 +1,57 @@
+// Similar to test-module-hooks-custom-conditions.mjs, but checking the
+// real require() instead of the re-invented one for imported CJS.
+'use strict';
+const common = require('../common');
+const { registerHooks } = require('node:module');
+const assert = require('node:assert');
+const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs');
+
+(async () => {
+  // Without hooks, the default condition is used.
+  assert.strictEqual(cjs('foo').result, 'default');
+  assert.strictEqual((await esm('foo')).result, 'default');
+
+  // Prepending 'foo' to the conditions array in the resolve hook should
+  // allow a CJS to be resolved with that condition.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        assert(Array.isArray(context.conditions));
+        context.conditions = ['foo', ...context.conditions];
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.strictEqual(cjs('foo/second').result, 'foo');
+    assert.strictEqual((await esm('foo/second')).result, 'foo');
+    hooks.deregister();
+  }
+
+  // Prepending 'foo-esm' to the conditions array in the resolve hook should
+  // allow a ESM to be resolved with that condition.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        assert(Array.isArray(context.conditions));
+        context.conditions = ['foo-esm', ...context.conditions];
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.strictEqual(cjs('foo/third').result, 'foo-esm');
+    assert.strictEqual((await esm('foo/third')).result, 'foo-esm');
+    hooks.deregister();
+  }
+
+  // Duplicating the 'foo' condition in the resolve hook should not change the result.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        assert(Array.isArray(context.conditions));
+        context.conditions = ['foo', ...context.conditions, 'foo'];
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.strictEqual(cjs('foo/fourth').result, 'foo');
+    assert.strictEqual((await esm('foo/fourth')).result, 'foo');
+    hooks.deregister();
+  }
+})().then(common.mustCall());

test/fixtures/es-modules/custom-condition/node_modules/foo/foo-esm.mjs

@@ -0,0 +1,70 @@
+// Check various special values of `conditions` in the context object
+// when using synchronous module hooks to override the loaders in a
+// CJS module.
+'use strict';
+const common = require('../common');
+const { registerHooks } = require('node:module');
+const assert = require('node:assert');
+const { cjs, esm } = require('../fixtures/es-modules/custom-condition/load.cjs');
+
+(async () => {
+  // Setting it to undefined would lead to the default conditions being used.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        context.conditions = undefined;
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.strictEqual(cjs('foo').result, 'default');
+    assert.strictEqual((await esm('foo')).result, 'default');
+    hooks.deregister();
+  }
+
+  // Setting it to an empty array would lead to the default conditions being used.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        context.conditions = [];
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.strictEqual(cjs('foo/second').result, 'default');
+    assert.strictEqual((await esm('foo/second')).result, 'default');
+    hooks.deregister();
+  }
+
+  // If the exports have no default export, it should error.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        context.conditions = [];
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.throws(() => cjs('foo/no-default'), {
+      code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
+    });
+    await assert.rejects(esm('foo/no-default'), {
+      code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
+    });
+    hooks.deregister();
+  }
+
+  // If the exports have no default export, it should error.
+  {
+    const hooks = registerHooks({
+      resolve(specifier, context, nextResolve) {
+        context.conditions = 'invalid';
+        return nextResolve(specifier, context);
+      },
+    });
+    assert.throws(() => cjs('foo/third'), {
+      code: 'ERR_INVALID_ARG_VALUE',
+    });
+    await assert.rejects(esm('foo/third'), {
+      code: 'ERR_INVALID_ARG_VALUE',
+    });
+    hooks.deregister();
+  }
+})().then(common.mustCall());