From ef5ce5ffa2cbf4d238af097b1eb6e72f5f86441c Mon Sep 17 00:00:00 2001
From: Joyee Cheung <joyeec9h3@gmail.com>
Date: Tue, 8 Oct 2024 22:26:13 +0200
Subject: [PATCH] module: check --experimental-require-module separately from
 detection

Previously we assumed if `--experimental-detect-module` is true, then
`--experimental-require-module` is true, which isn't the case, as
the two can be enabled/disabled separately. This patch fixes the
checks so `--no-experimental-require-module` is still effective when
`--experimental-detect-module` is enabled.

Drive-by: make the assertion messages more informative and remove
obsolete TODO about allowing TLA in entrypoints handled by
require(esm).

PR-URL: https://github.com/nodejs/node/pull/55250
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Jacob Smith <jacob@frende.me>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
---
 lib/internal/modules/cjs/loader.js            | 16 ++++++++++++----
 ...t-disable-require-module-with-detection.js | 19 +++++++++++++++++++
 test/fixtures/es-modules/loose.js             |  4 +++-
 3 files changed, 34 insertions(+), 5 deletions(-)
 create mode 100644 test/es-module/test-disable-require-module-with-detection.js

diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js
index 752dfe670ed96c..f4244abe2f4fa3 100644
--- a/lib/internal/modules/cjs/loader.js
+++ b/lib/internal/modules/cjs/loader.js
@@ -1437,7 +1437,7 @@ function loadESMFromCJS(mod, filename) {
  * @param {'commonjs'|undefined} format Intended format of the module.
  */
 function wrapSafe(filename, content, cjsModuleInstance, format) {
-  assert(format !== 'module');  // ESM should be handled in loadESMFromCJS().
+  assert(format !== 'module', 'ESM should be handled in loadESMFromCJS()');
   const hostDefinedOptionId = vm_dynamic_import_default_internal;
   const importModuleDynamically = vm_dynamic_import_default_internal;
   if (patched) {
@@ -1467,7 +1467,17 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {
     };
   }
 
-  const shouldDetectModule = (format !== 'commonjs' && getOptionValue('--experimental-detect-module'));
+  let shouldDetectModule = false;
+  if (format !== 'commonjs') {
+    if (cjsModuleInstance?.[kIsMainSymbol]) {
+      // For entry points, format detection is used unless explicitly disabled.
+      shouldDetectModule = getOptionValue('--experimental-detect-module');
+    } else {
+      // For modules being loaded by `require()`, if require(esm) is disabled,
+      // don't try to reparse to detect format and just throw for ESM syntax.
+      shouldDetectModule = getOptionValue('--experimental-require-module');
+    }
+  }
   const result = compileFunctionForCJSLoader(content, filename, false /* is_sea_main */, shouldDetectModule);
 
   // Cache the source map for the module if present.
@@ -1504,8 +1514,6 @@ Module.prototype._compile = function(content, filename, format) {
     }
   }
 
-  // TODO(joyeecheung): when the module is the entry point, consider allowing TLA.
-  // Only modules being require()'d really need to avoid TLA.
   if (format === 'module') {
     // Pass the source into the .mjs extension handler indirectly through the cache.
     this[kModuleSource] = content;
diff --git a/test/es-module/test-disable-require-module-with-detection.js b/test/es-module/test-disable-require-module-with-detection.js
new file mode 100644
index 00000000000000..3ac5ec82171587
--- /dev/null
+++ b/test/es-module/test-disable-require-module-with-detection.js
@@ -0,0 +1,19 @@
+// Flags: --no-experimental-require-module
+'use strict';
+
+// Tests that --experimental-require-module is not implied by --experimental-detect-module
+// and is checked independently.
+require('../common');
+const assert = require('assert');
+
+// Check that require() still throws SyntaxError for an ambiguous module that's detected to be ESM.
+// TODO(joyeecheung): now that --experimental-detect-module is unflagged, it makes more sense
+// to either throw ERR_REQUIRE_ESM for require() of detected ESM instead, or add a hint about the
+// use of require(esm) to the SyntaxError.
+
+assert.throws(
+  () => require('../fixtures/es-modules/loose.js'),
+  {
+    name: 'SyntaxError',
+    message: /Unexpected token 'export'/
+  });
diff --git a/test/fixtures/es-modules/loose.js b/test/fixtures/es-modules/loose.js
index 69147a3b8ca027..c0d85f7eab9a2b 100644
--- a/test/fixtures/es-modules/loose.js
+++ b/test/fixtures/es-modules/loose.js
@@ -1,3 +1,5 @@
-// This file can be run or imported only if `--experimental-default-type=module` is set.
+// This file can be run or imported only if `--experimental-default-type=module` is set
+// or `--experimental-detect-module` is not disabled. If it's loaded by
+// require(), then `--experimental-require-module` must not be disabled.
 export default 'module';
 console.log('executed');