esm: fix relative require in non-file CJS source

When a load hook provides the source of a CommonJS module, the require
calls in that source are meant to go through the ESM loader so that the
registered hooks apply to them, as documented for the load hook.

That did not happen when the module had a URL that is not a file:. The
require function built for those modules called
Module._resolveFilename() before handing the specifier to the ESM
resolver, only to look at the extension of the resulting path and decide
whether to treat the request as JSON or as a native addon. Because the
CJS resolver works on file paths, it cannot resolve a relative specifier
against a referrer such as custom:cjs, so it threw MODULE_NOT_FOUND and
the ESM resolution below it was never reached. The resolve hook was not
called at all.

Keep that lookup, since the JSON and .node branches depend on it, but
let it fail when the referrer is not a file and leave the specifier for
the ESM resolver to handle.

A specifier that the hooks do not claim now fails with
ERR_UNSUPPORTED_RESOLVE_REQUEST, which reports that the referrer scheme
is not hierarchical, instead of MODULE_NOT_FOUND coming from the CJS
resolver.

Signed-off-by: Avocado <ujubongbong@gmail.com>

Author: zeexzeex

lib/internal/modules/esm/translators.js

@@ -129,19 +129,32 @@ function loadCJSModuleWithSpecialRequire(module, source, url, filename, isMain,
     let importAttributes = kEmptyObject;
     if (!StringPrototypeStartsWith(specifier, 'node:') && !BuiltinModule.normalizeRequirableId(specifier)) {
       // TODO: do not depend on the monkey-patchable CJS loader here.
-      const path = CJSModule._resolveFilename(specifier, module);
-      switch (extname(path)) {
-        case '.json':
-          importAttributes = { __proto__: null, type: 'json' };
-          break;
-        case '.node':
-          // If it gets here in the translators, the hooks must have already been invoked
-          // in the loader. Skip them in the synthetic module evaluation step.
-          return wrapModuleLoad(specifier, module, false, kShouldSkipModuleHooks);
-        default:
-            // fall through
+      // The CJS resolver works on file paths, so it cannot resolve a specifier whose
+      // referrer is not a file - that happens when the source of this module comes from
+      // a load hook that gave it a non-file URL. Leave the specifier alone in that case
+      // and let the ESM resolver below run the hooks on it.
+      let path;
+      try {
+        path = CJSModule._resolveFilename(specifier, module);
+      } catch (err) {
+        if (err?.code !== 'MODULE_NOT_FOUND' || StringPrototypeStartsWith(url, 'file:')) {
+          throw err;
+        }
+      }
+      if (path !== undefined) {
+        switch (extname(path)) {
+          case '.json':
+            importAttributes = { __proto__: null, type: 'json' };
+            break;
+          case '.node':
+            // If it gets here in the translators, the hooks must have already been invoked
+            // in the loader. Skip them in the synthetic module evaluation step.
+            return wrapModuleLoad(specifier, module, false, kShouldSkipModuleHooks);
+          default:
+              // fall through
+        }
+        specifier = `${pathToFileURL(path)}`;
       }
-      specifier = `${pathToFileURL(path)}`;
     }
 
     // NOTE: This re-invented require() is only used on the loader-hook worker thread.

test/es-module/test-esm-loader-cjs-source-relative-require.mjs

@@ -0,0 +1,17 @@
+// Flags: --experimental-loader ./test/fixtures/es-module-loaders/non-file-cjs-source.mjs
+import '../common/index.mjs';
+import assert from 'node:assert';
+
+// When a load hook provides the source of a CommonJS module, the `require` calls
+// that source makes are resolved by the ESM loader, so registered hooks apply to
+// them. That has to keep working when the module has a URL that is not a file:
+// the CJS resolver cannot resolve a relative specifier against such a referrer.
+
+const { default: fromHookedRequire } = await import('custom:entry');
+assert.strictEqual(fromHookedRequire, 'loaded through the hooks');
+
+// When the hooks do not claim the specifier either, the failure has to come from
+// the ESM resolver rather than from the CJS one.
+await assert.rejects(import('custom:missing-dep'), {
+  code: 'ERR_UNSUPPORTED_RESOLVE_REQUEST',
+});

test/fixtures/es-module-loaders/non-file-cjs-source.mjs

@@ -0,0 +1,25 @@
+// Serves CommonJS sources under a non-file URL scheme, so that the `require`
+// calls they contain have a referrer that the CJS resolver cannot handle.
+
+const sources = {
+  'custom:entry': 'module.exports = require("./dep");',
+  'custom:dep': 'module.exports = "loaded through the hooks";',
+  'custom:missing-dep': 'module.exports = require("./no-such-dep");',
+};
+
+export function resolve(specifier, context, next) {
+  if (specifier in sources) {
+    return { shortCircuit: true, url: specifier };
+  }
+  if (specifier === './dep') {
+    return { shortCircuit: true, url: 'custom:dep' };
+  }
+  return next(specifier, context);
+}
+
+export function load(url, context, next) {
+  if (url in sources) {
+    return { shortCircuit: true, format: 'commonjs', source: sources[url] };
+  }
+  return next(url, context);
+}