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: nodejs#55250
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: Jacob Smith <[email protected]>
Reviewed-By: Rafael Gonzaga <[email protected]>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -1404,7 +1404,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) {
@@ -1434,7 +1434,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.
@@ -1471,8 +1481,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;

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'/
+  });

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');