From fbdf291bb9b375275903f121d4ddc6fb38afb616 Mon Sep 17 00:00:00 2001
From: Antoine du Hamel <duhamelantoine1995@gmail.com>
Date: Wed, 29 Nov 2023 10:12:05 +0100
Subject: [PATCH] esm: fallback to `getSource` when `load` returns nullish
 `source`

When using the Modules Customization Hooks API to load CommonJS modules,
we want to support the returned value of `defaultLoad` which must be
nullish to preserve backward compatibility. This can be achieved by
fetching the source from the translator.

PR-URL: https://github.com/nodejs/node/pull/50825
Fixes: https://github.com/nodejs/node/issues/50435
Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
---
 lib/internal/modules/esm/load.js         |  1 +
 lib/internal/modules/esm/translators.js  | 35 ++++++++++++++++--------
 test/es-module/test-esm-loader-hooks.mjs | 35 ++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 12 deletions(-)

diff --git a/lib/internal/modules/esm/load.js b/lib/internal/modules/esm/load.js
index 70d1b7fe83fa35..5239bc8ed883a5 100644
--- a/lib/internal/modules/esm/load.js
+++ b/lib/internal/modules/esm/load.js
@@ -262,5 +262,6 @@ function throwUnknownModuleFormat(url, format) {
 module.exports = {
   defaultLoad,
   defaultLoadSync,
+  getSourceSync,
   throwUnknownModuleFormat,
 };
diff --git a/lib/internal/modules/esm/translators.js b/lib/internal/modules/esm/translators.js
index 7a62615cfe4210..9976b819f266c6 100644
--- a/lib/internal/modules/esm/translators.js
+++ b/lib/internal/modules/esm/translators.js
@@ -31,6 +31,7 @@ function lazyTypes() {
 }
 
 const { containsModuleSyntax } = internalBinding('contextify');
+const { BuiltinModule } = require('internal/bootstrap/realm');
 const assert = require('internal/assert');
 const { readFileSync } = require('fs');
 const { dirname, extname, isAbsolute } = require('path');
@@ -58,6 +59,17 @@ const asyncESM = require('internal/process/esm_loader');
 const { emitWarningSync } = require('internal/process/warning');
 const { internalCompileFunction } = require('internal/vm');
 
+// Lazy-loading to avoid circular dependencies.
+let getSourceSync;
+/**
+ * @param {Parameters<typeof import('./load').getSourceSync>[0]} url
+ * @returns {ReturnType<typeof import('./load').getSourceSync>}
+ */
+function getSource(url) {
+  getSourceSync ??= require('internal/modules/esm/load').getSourceSync;
+  return getSourceSync(url);
+}
+
 /** @type {import('deps/cjs-module-lexer/lexer.js').parse} */
 let cjsParse;
 /**
@@ -225,21 +237,19 @@ function loadCJSModule(module, source, url, filename) {
   // eslint-disable-next-line func-name-matching,func-style
   const requireFn = function require(specifier) {
     let importAttributes = kEmptyObject;
-    if (!StringPrototypeStartsWith(specifier, 'node:')) {
+    if (!StringPrototypeStartsWith(specifier, 'node:') && !BuiltinModule.normalizeRequirableId(specifier)) {
       // TODO: do not depend on the monkey-patchable CJS loader here.
       const path = CJSModule._resolveFilename(specifier, module);
-      if (specifier !== path) {
-        switch (extname(path)) {
-          case '.json':
-            importAttributes = { __proto__: null, type: 'json' };
-            break;
-          case '.node':
-            return CJSModule._load(specifier, module);
-          default:
+      switch (extname(path)) {
+        case '.json':
+          importAttributes = { __proto__: null, type: 'json' };
+          break;
+        case '.node':
+          return CJSModule._load(specifier, module);
+        default:
             // fall through
-        }
-        specifier = `${pathToFileURL(path)}`;
       }
+      specifier = `${pathToFileURL(path)}`;
     }
     const job = asyncESM.esmLoader.getModuleJobSync(specifier, url, importAttributes);
     job.runSync();
@@ -276,7 +286,8 @@ function createCJSModuleWrap(url, source, isMain, loadCJS = loadCJSModule) {
   debug(`Translating CJSModule ${url}`);
 
   const filename = StringPrototypeStartsWith(url, 'file://') ? fileURLToPath(url) : url;
-  source = stringify(source);
+  // In case the source was not provided by the `load` step, we need fetch it now.
+  source = stringify(source ?? getSource(new URL(url)).source);
 
   const { exportNames, module } = cjsPreparseModuleExports(filename, source);
   cjsCache.set(url, module);
diff --git a/test/es-module/test-esm-loader-hooks.mjs b/test/es-module/test-esm-loader-hooks.mjs
index 6544c05ec93f8e..d5fee004392b44 100644
--- a/test/es-module/test-esm-loader-hooks.mjs
+++ b/test/es-module/test-esm-loader-hooks.mjs
@@ -736,4 +736,39 @@ describe('Loader hooks', { concurrency: true }, () => {
     assert.strictEqual(code, 0);
     assert.strictEqual(signal, null);
   });
+
+  it('should handle mixed of opt-in modules and non-opt-in ones', async () => {
+    const { code, signal, stdout, stderr } = await spawnPromisified(execPath, [
+      '--no-warnings',
+      '--experimental-loader',
+      `data:text/javascript,const fixtures=${JSON.stringify(fixtures.path('empty.js'))};export ${
+        encodeURIComponent(function resolve(s, c, n) {
+          if (s.endsWith('entry-point')) {
+            return {
+              shortCircuit: true,
+              url: 'file:///c:/virtual-entry-point',
+              format: 'commonjs',
+            };
+          }
+          return n(s, c);
+        })
+      }export ${
+        encodeURIComponent(async function load(u, c, n) {
+          if (u === 'file:///c:/virtual-entry-point') {
+            return {
+              shortCircuit: true,
+              source: `"use strict";require(${JSON.stringify(fixtures)});console.log("Hello");`,
+              format: 'commonjs',
+            };
+          }
+          return n(u, c);
+        })}`,
+      'entry-point',
+    ]);
+
+    assert.strictEqual(stderr, '');
+    assert.strictEqual(stdout, 'Hello\n');
+    assert.strictEqual(code, 0);
+    assert.strictEqual(signal, null);
+  });
 });