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: #59011Fixes: #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}
  */
 functionresolveExports(nmPath,request,conditions){
@@ -1068,17 +1070,30 @@ function resolveForCJSWithHooks(specifier, parent, isMain){
 functiondefaultResolve(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.
+letconditionSet;
+constconditions=context.conditions;
+if(conditions!==undefined&&conditions!==getCjsConditionsArray()){
+if(!ArrayIsArray(conditions)){
+thrownewERR_INVALID_ARG_VALUE('context.conditions',conditions,
+'expected an array');
+}
+conditionSet=newSafeSet(conditions);
+}else{
+conditionSet=getCjsConditions();
+}
 defaultResolvedFilename=defaultResolveImpl(specifier,parent,isMain,{
 __proto__: null,
-conditions: context.conditions,
+conditions: conditionSet,
 });
 
 defaultResolvedURL=convertCJSFilenameToURL(defaultResolvedFilename);
 return{__proto__: null,url: defaultResolvedURL};
 }
 
 constresolveResult=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?
 constloadResult=loadWithHooks(url,format||'builtin',/* importAttributes */undefined,
-getCjsConditions(),getDefaultLoad(url,id));
+getCjsConditionsArray(),getDefaultLoad(url,id));
 if(loadResult.format&&loadResult.format!=='builtin'){
 returnundefined;// 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);
 }
 
-constloadResult=loadWithHooks(mod[kURL],mod[kFormat],/* importAttributes */undefined,getCjsConditions(),
+constloadResult=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.
 returnthis.#customizations.resolve(specifier,parentURL,importAttributes);
 }
-returnthis.#cachedDefaultResolve(specifier,parentURL,importAttributes);
+returnthis.#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;
 constrequestKey=this.#resolveCache.serializeKey(specifier,importAttributes);
 constcachedResult=this.#resolveCache.get(requestKey,parentURL);
 if(cachedResult!=null){
 returncachedResult;
 }
-constresult=this.defaultResolve(specifier,parentURL,importAttributes);
+defaultResolve??=require('internal/modules/esm/resolve').defaultResolve;
+constresult=defaultResolve(specifier,context);
 this.#resolveCache.set(requestKey,parentURL,result);
 returnresult;
 }
@@ -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){
 returnthis.#customizations.resolveSync(specifier,context.parentURL,context.importAttributes);
 }
-returnthis.#cachedDefaultResolve(specifier,context.parentURL,context.importAttributes);
+returnthis.#cachedDefaultResolve(specifier,context);
 }
 
 /**
@@ -784,26 +791,12 @@ class ModuleLoader{
 returnresolveWithHooks(specifier,parentURL,importAttributes,this.#defaultConditions,
 this.#resolveAndMaybeBlockOnLoaderThread.bind(this));
 }
-returnthis.#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;
-
-constcontext={
+returnthis.#resolveAndMaybeBlockOnLoaderThread(specifier,{
 __proto__: null,
 conditions: this.#defaultConditions,
- importAttributes,
  parentURL,
-};
-
-returndefaultResolve(originalSpecifier,context);
+ importAttributes,
+});
 }
 
 /**

lib/internal/modules/helpers.js

@@ -66,6 +66,9 @@ function toRealPath(requestPath){
 
 /** @type{Set<string>} */
 letcjsConditions;
+/** @type{string[]} */
+letcjsConditionsArray;
+
 /**
  * Define the conditions that apply to the CommonJS loader.
  * @returns{void}
@@ -75,15 +78,17 @@ function initializeCjsConditions(){
 constnoAddons=getOptionValue('--no-addons');
 constaddonConditions=noAddons ? [] : ['node-addons'];
 // TODO: Use this set when resolving pkg#exports conditions in loader.js.
-cjsConditions=newSafeSet([
+cjsConditionsArray=[
 'require',
 'node',
  ...addonConditions,
  ...userConditions,
-]);
+];
 if(getOptionValue('--experimental-require-module')){
-cjsConditions.add('module-sync');
+cjsConditionsArray.push('module-sync');
 }
+ObjectFreeze(cjsConditionsArray);
+cjsConditions=newSafeSet(cjsConditionsArray);
 }
 
 /**
@@ -97,6 +102,13 @@ function getCjsConditions(){
 returncjsConditions;
 }
 
+functiongetCjsConditionsArray(){
+if(cjsConditionsArray===undefined){
+initializeCjsConditions();
+}
+returncjsConditionsArray;
+}
+
 /**
  * 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';
+constcommon=require('../common');
+const{ registerHooks }=require('node:module');
+constassert=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((awaitesm('foo')).result,'default');
+
+// Prepending 'foo' to the conditions array in the resolve hook should
+// allow a CJS to be resolved with that condition.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+assert(Array.isArray(context.conditions));
+context.conditions=['foo', ...context.conditions];
+returnnextResolve(specifier,context);
+},
+});
+assert.strictEqual(cjs('foo/second').result,'foo');
+assert.strictEqual((awaitesm('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.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+assert(Array.isArray(context.conditions));
+context.conditions=['foo-esm', ...context.conditions];
+returnnextResolve(specifier,context);
+},
+});
+assert.strictEqual(cjs('foo/third').result,'foo-esm');
+assert.strictEqual((awaitesm('foo/third')).result,'foo-esm');
+hooks.deregister();
+}
+
+// Duplicating the 'foo' condition in the resolve hook should not change the result.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+assert(Array.isArray(context.conditions));
+context.conditions=['foo', ...context.conditions,'foo'];
+returnnextResolve(specifier,context);
+},
+});
+assert.strictEqual(cjs('foo/fourth').result,'foo');
+assert.strictEqual((awaitesm('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';
+constcommon=require('../common');
+const{ registerHooks }=require('node:module');
+constassert=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.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+context.conditions=undefined;
+returnnextResolve(specifier,context);
+},
+});
+assert.strictEqual(cjs('foo').result,'default');
+assert.strictEqual((awaitesm('foo')).result,'default');
+hooks.deregister();
+}
+
+// Setting it to an empty array would lead to the default conditions being used.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+context.conditions=[];
+returnnextResolve(specifier,context);
+},
+});
+assert.strictEqual(cjs('foo/second').result,'default');
+assert.strictEqual((awaitesm('foo/second')).result,'default');
+hooks.deregister();
+}
+
+// If the exports have no default export, it should error.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+context.conditions=[];
+returnnextResolve(specifier,context);
+},
+});
+assert.throws(()=>cjs('foo/no-default'),{
+code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
+});
+awaitassert.rejects(esm('foo/no-default'),{
+code: 'ERR_PACKAGE_PATH_NOT_EXPORTED',
+});
+hooks.deregister();
+}
+
+// If the exports have no default export, it should error.
+{
+consthooks=registerHooks({
+resolve(specifier,context,nextResolve){
+context.conditions='invalid';
+returnnextResolve(specifier,context);
+},
+});
+assert.throws(()=>cjs('foo/third'),{
+code: 'ERR_INVALID_ARG_VALUE',
+});
+awaitassert.rejects(esm('foo/third'),{
+code: 'ERR_INVALID_ARG_VALUE',
+});
+hooks.deregister();
+}
+})().then(common.mustCall());