Repository: nodejs/node
Pull request: 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).

<!--
Before submitting a pull request, please read:

- the CONTRIBUTING guide at https://github.com/nodejs/node/blob/HEAD/CONTRIBUTING.md
- the commit message formatting guidelines at
  https://github.com/nodejs/node/blob/HEAD/doc/contributing/pull-requests.md#commit-message-guidelines

For code changes:
1. Include tests for any bug fixes or new features.
2. Update documentation if relevant.
3. Ensure that `make -j4 test` (UNIX), or `vcbuild test` (Windows) passes.

If you believe this PR should be highlighted in the Node.js CHANGELOG
please add the `notable-change` label.

Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.
-->


Changed files:
filename: lib/internal/modules/cjs/loader.js
patch: @@ -1449,7 +1449,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) {
@@ -1479,7 +1479,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.
@@ -1509,8 +1519,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;

filename: test/es-module/test-disable-require-module-with-detection.js
patch: @@ -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'/
+  });

filename: test/fixtures/es-modules/loose.js
patch: @@ -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');