module: fix conditions override in synchronous resolve hooks

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,
@@ -635,6 +637,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 {Set<string>} conditions The conditions to use for resolution.
  */
 function resolveExports(nmPath, request, conditions) {
   // The implementation's behavior is meant to mirror resolution in ESM.
@@ -1043,17 +1046,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;
 
@@ -1129,7 +1145,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.
     }
@@ -1280,7 +1296,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 Conditions used for resolution.
  */
 Module._resolveFilename = function(request, parent, isMain, options) {
   if (BuiltinModule.normalizeRequirableId(request)) {
@@ -1723,7 +1739,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

@@ -705,24 +705,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;
   }
@@ -750,14 +756,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);
   }
 
   /**
@@ -780,25 +787,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.
-   */
-  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

@@ -65,6 +65,9 @@ function toRealPath(requestPath) {
 
 /** @type {Set<string>} */
 let cjsConditions;
+/** @type {string[]} */
+let cjsConditionsArray;
+
 /**
  * Define the conditions that apply to the CommonJS loader.
  */
@@ -73,15 +76,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);
 }
 
 /**
@@ -94,6 +99,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
@@ -408,6 +420,7 @@ module.exports = {
   flushCompileCache,
   getBuiltinModule,
   getCjsConditions,
+  getCjsConditionsArray,
   getCompileCacheDir,
   initializeCjsConditions,
   loadBuiltinModule,

test/fixtures/es-modules/custom-condition/load.cjs

@@ -0,0 +1,7 @@
+exports.cjs = function(key) {
+  return require(key);
+};
+exports.esm = function(key) {
+  return import(key);
+};
+

test/module-hooks/test-module-hooks-custom-conditions-cjs.js

@@ -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();
+  }
+})().catch(common.mustNotCall());

test/module-hooks/test-module-hooks-custom-conditions-special-values.js

@@ -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();
+  }
+})().catch(common.mustNotCall());