fixup! module: run require.resolve through module.registerHooks()

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -1034,43 +1034,63 @@ function getExportsForCircularRequire(module) {
   return module.exports;
 }
 
+
 /**
- * Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
- * if necessary.
+ * Wraps result of Module._resolveFilename to include additional fields for hooks.
+ * See resolveForCJSWithHooks.
  * @param {string} specifier
  * @param {Module|undefined} parent
  * @param {boolean} isMain
- * @param {boolean} shouldSkipModuleHooks
- * @param {object} [resolveOptions] Options from require.resolve().
- * @param {string[]} [resolveOptions.paths] Paths to search for modules in.
+ * @param {ResolveFilenameOptions} options
  * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
  */
-function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks, resolveOptions) {
-  let defaultResolvedURL;
-  let defaultResolvedFilename;
-  let format;
-
-  function defaultResolveImpl(specifier, parent, isMain, options) {
-    // For backwards compatibility, when encountering requests starting with node:,
-    // throw ERR_UNKNOWN_BUILTIN_MODULE on failure or return the normalized ID on success
-    // without going into Module._resolveFilename.
-    let normalized;
-    if (StringPrototypeStartsWith(specifier, 'node:')) {
-      normalized = BuiltinModule.normalizeRequirableId(specifier);
-      if (!normalized) {
-        throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
-      }
-      defaultResolvedURL = specifier;
-      format = 'builtin';
-      return normalized;
+function wrapResolveFilename(specifier, parent, isMain, options) {
+  const filename = Module._resolveFilename(specifier, parent, isMain, options).toString();
+  return { __proto__: null, url: undefined, format: undefined, filename };
+}
+
+/**
+ * See resolveForCJSWithHooks.
+ * @param {string} specifier
+ * @param {Module|undefined} parent
+ * @param {boolean} isMain
+ * @param {ResolveFilenameOptions} options
+ * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
+ */
+function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) {
+  // For backwards compatibility, when encountering requests starting with node:,
+  // throw ERR_UNKNOWN_BUILTIN_MODULE on failure or return the normalized ID on success
+  // without going into Module._resolveFilename.
+  let normalized;
+  if (StringPrototypeStartsWith(specifier, 'node:')) {
+    normalized = BuiltinModule.normalizeRequirableId(specifier);
+    if (!normalized) {
+      throw new ERR_UNKNOWN_BUILTIN_MODULE(specifier);
     }
-    return Module._resolveFilename(specifier, parent, isMain, options).toString();
+    return { __proto__: null, url: specifier, format: 'builtin', filename: normalized };
   }
+  return wrapResolveFilename(specifier, parent, isMain, options);
+}
 
+/**
+ * Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
+ * if necessary.
+ * @param {string} specifier
+ * @param {Module|undefined} parent
+ * @param {boolean} isMain
+ * @param {object} internalResolveOptions
+ * @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks.
+ * @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve().
+ *   Only used when it comes from require.resolve().
+ * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
+ */
+function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) {
+  const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions;
+  const defaultResolveImpl = requireResolveOptions ?
+    wrapResolveFilename : defaultResolveImplForCJSLoading;
   // Fast path: no hooks, just return simple results.
   if (!resolveHooks.length || shouldSkipModuleHooks) {
-    const filename = defaultResolveImpl(specifier, parent, isMain, resolveOptions);
-    return { __proto__: null, url: defaultResolvedURL, filename, format };
+    return defaultResolveImpl(specifier, parent, isMain, requireResolveOptions);
   }
 
   // Slow path: has hooks, do the URL conversions and invoke hooks with contexts.
@@ -1099,28 +1119,25 @@ function resolveForCJSWithHooks(specifier, parent, isMain, shouldSkipModuleHooks
     } else {
       conditionSet = getCjsConditions();
     }
-    defaultResolvedFilename = defaultResolveImpl(specifier, parent, isMain, {
+
+    const result = defaultResolveImpl(specifier, parent, isMain, {
       __proto__: null,
-      paths: resolveOptions?.paths,
+      paths: requireResolveOptions?.paths,
       conditions: conditionSet,
     });
+    // If the default resolver does not return a URL, convert it for the public API.
+    result.url ??= convertCJSFilenameToURL(result.filename);
 
-    defaultResolvedURL = convertCJSFilenameToURL(defaultResolvedFilename);
-    return { __proto__: null, url: defaultResolvedURL };
+    // Remove filename because it's not part of the public API.
+    // TODO(joyeecheung): maybe expose it in the public API to avoid re-conversion for users too.
+    return { __proto__: null, url: result.url, format: result.format };
   }
 
   const resolveResult = resolveWithHooks(specifier, parentURL, /* importAttributes */ undefined,
                                          getCjsConditionsArray(), defaultResolve);
-  const { url } = resolveResult;
-  format = resolveResult.format;
-
-  let filename;
-  if (url === defaultResolvedURL) {  // Not overridden, skip the re-conversion.
-    filename = defaultResolvedFilename;
-  } else {
-    filename = convertURLToCJSFilename(url);
-  }
-
+  const { url, format } = resolveResult;
+  // Convert the URL from the hook chain back to a filename for internal use.
+  const filename = convertURLToCJSFilename(url);
   const result = { __proto__: null, url, format, filename, parentURL };
   debug('resolveForCJSWithHooks', specifier, parent?.id, isMain, shouldSkipModuleHooks, '->', result);
   return result;
@@ -1215,10 +1232,10 @@ function loadBuiltinWithHooks(id, url, format) {
  * @param {string} request Specifier of module to load via `require`
  * @param {Module} parent Absolute path of the module importing the child
  * @param {boolean} isMain Whether the module is the main entry point
- * @param {object|undefined} options Additional options for loading the module
+ * @param {object|undefined} internalResolveOptions Additional options for loading the module
  * @returns {object}
  */
-Module._load = function(request, parent, isMain, options = kEmptyObject) {
+Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) {
   let relResolveCacheIdentifier;
   if (parent) {
     debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1241,7 +1258,7 @@ Module._load = function(request, parent, isMain, options = kEmptyObject) {
     }
   }
 
-  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, options.shouldSkipModuleHooks);
+  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, internalResolveOptions);
   let { format } = resolveResult;
   const { url, filename } = resolveResult;
 

lib/internal/modules/esm/translators.js

@@ -94,6 +94,8 @@ translators.set('module', function moduleStrategy(url, translateContext, parentU
 
 const { requestTypes: { kRequireInImportedCJS } } = require('internal/modules/esm/utils');
 const kShouldSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: true };
+const kShouldNotSkipModuleHooks = { __proto__: null, shouldSkipModuleHooks: false };
+
 /**
  * Loads a CommonJS module via the ESM Loader sync CommonJS translator.
  * This translator creates its own version of the `require` function passed into CommonJS modules.
@@ -164,9 +166,11 @@ function loadCJSModule(module, source, url, filename, isMain) {
   };
   setOwnProperty(requireFn, 'resolve', function resolve(specifier) {
     if (!StringPrototypeStartsWith(specifier, 'node:')) {
-      const { filename } = resolveForCJSWithHooks(specifier, module, false, false);
+      const {
+        filename, url: resolvedURL,
+      } = resolveForCJSWithHooks(specifier, module, false, kShouldNotSkipModuleHooks);
       if (specifier !== filename) {
-        specifier = `${pathToFileURL(filename)}`;
+        specifier = resolvedURL ?? `${pathToFileURL(filename)}`;
       }
     }
 
@@ -402,7 +406,7 @@ function cjsPreparseModuleExports(filename, source, format) {
       let resolved;
       let format;
       try {
-        ({ format, filename: resolved } = resolveForCJSWithHooks(reexport, module, false));
+        ({ format, filename: resolved } = resolveForCJSWithHooks(reexport, module, false, kShouldNotSkipModuleHooks));
       } catch (e) {
         debug(`Failed to resolve '${reexport}', skipping`, e);
         continue;

lib/internal/modules/helpers.js

@@ -200,8 +200,21 @@ function makeRequireFunction(mod) {
   function resolve(request, options) {
     validateString(request, 'request');
     const { resolveForCJSWithHooks } = lazyCJSLoader();
-    const { filename } = resolveForCJSWithHooks(
-      request, mod, /* isMain */ false, /* shouldSkipModuleHooks */ false, options);
+    // require.resolve() has different behaviors from the internal resolution used by
+    // Module._load:
+    // 1. When the request resolves to a non-existent built-in, it throws MODULE_NOT_FOUND
+    //   instead of UNKNOWN_BUILTIN_MODULE. This is handled by resolveForCJSWithHooks.
+    // 2. If the request is a prefixed built-in, the returned value is also prefixed. This
+    //   is handled below.
+    const { filename, url } = resolveForCJSWithHooks(
+      request, mod, /* isMain */ false, {
+        __proto__: null,
+        shouldSkipModuleHooks: false,
+        requireResolveOptions: options ?? {},
+      });
+    if (url === request && StringPrototypeStartsWith(request, 'node:')) {
+      return url;
+    }
     return filename;
   }
 

test/parallel/test-repl.js

@@ -608,17 +608,18 @@ const errorTests = [
   // access to internal modules without the --expose-internals flag.
   {
     // Shrink the stack trace to avoid having to update this test whenever the
-    // implementation of require() changes. It's set to 4 because somehow setting it
+    // implementation of require() changes. It's set to 5 because somehow setting it
     // to a lower value breaks the error formatting and the message becomes
     // "Uncaught [Error...", which is probably a bug(?).
-    send: 'Error.stackTraceLimit = 4; require("internal/repl")',
+    send: 'Error.stackTraceLimit = 5; require("internal/repl")',
     expect: [
       /^Uncaught Error: Cannot find module 'internal\/repl'/,
       /^Require stack:/,
       /^- <repl>/,  // This just tests MODULE_NOT_FOUND so let's skip the stack trace
       /^ {4}at .*/, // Some stack frame that we have to capture otherwise error message is buggy.
       /^ {4}at .*/, // Some stack frame that we have to capture otherwise error message is buggy.
       /^ {4}at .*/, // Some stack frame that we have to capture otherwise error message is buggy.
+      /^ {4}at .*/, // Some stack frame that we have to capture otherwise error message is buggy.
       "  code: 'MODULE_NOT_FOUND',",
       "  requireStack: [ '<repl>' ]",
       '}',