Skip to content

Commit 3fb7426

Browse files
authored
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: #55250 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Jacob Smith <jacob@frende.me> Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
1 parent 3a42085 commit 3fb7426

File tree

3 files changed

+34
-5
lines changed

3 files changed

+34
-5
lines changed

lib/internal/modules/cjs/loader.js

+12-4
Original file line numberDiff line numberDiff line change
@@ -1449,7 +1449,7 @@ function loadESMFromCJS(mod, filename) {
14491449
* @param {'commonjs'|undefined} format Intended format of the module.
14501450
*/
14511451
function wrapSafe(filename, content, cjsModuleInstance, format) {
1452-
assert(format !== 'module'); // ESM should be handled in loadESMFromCJS().
1452+
assert(format !== 'module', 'ESM should be handled in loadESMFromCJS()');
14531453
const hostDefinedOptionId = vm_dynamic_import_default_internal;
14541454
const importModuleDynamically = vm_dynamic_import_default_internal;
14551455
if (patched) {
@@ -1479,7 +1479,17 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {
14791479
};
14801480
}
14811481

1482-
const shouldDetectModule = (format !== 'commonjs' && getOptionValue('--experimental-detect-module'));
1482+
let shouldDetectModule = false;
1483+
if (format !== 'commonjs') {
1484+
if (cjsModuleInstance?.[kIsMainSymbol]) {
1485+
// For entry points, format detection is used unless explicitly disabled.
1486+
shouldDetectModule = getOptionValue('--experimental-detect-module');
1487+
} else {
1488+
// For modules being loaded by `require()`, if require(esm) is disabled,
1489+
// don't try to reparse to detect format and just throw for ESM syntax.
1490+
shouldDetectModule = getOptionValue('--experimental-require-module');
1491+
}
1492+
}
14831493
const result = compileFunctionForCJSLoader(content, filename, false /* is_sea_main */, shouldDetectModule);
14841494

14851495
// Cache the source map for the module if present.
@@ -1509,8 +1519,6 @@ Module.prototype._compile = function(content, filename, format) {
15091519
}
15101520
}
15111521

1512-
// TODO(joyeecheung): when the module is the entry point, consider allowing TLA.
1513-
// Only modules being require()'d really need to avoid TLA.
15141522
if (format === 'module') {
15151523
// Pass the source into the .mjs extension handler indirectly through the cache.
15161524
this[kModuleSource] = content;
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
// Flags: --no-experimental-require-module
2+
'use strict';
3+
4+
// Tests that --experimental-require-module is not implied by --experimental-detect-module
5+
// and is checked independently.
6+
require('../common');
7+
const assert = require('assert');
8+
9+
// Check that require() still throws SyntaxError for an ambiguous module that's detected to be ESM.
10+
// TODO(joyeecheung): now that --experimental-detect-module is unflagged, it makes more sense
11+
// to either throw ERR_REQUIRE_ESM for require() of detected ESM instead, or add a hint about the
12+
// use of require(esm) to the SyntaxError.
13+
14+
assert.throws(
15+
() => require('../fixtures/es-modules/loose.js'),
16+
{
17+
name: 'SyntaxError',
18+
message: /Unexpected token 'export'/
19+
});

test/fixtures/es-modules/loose.js

+3-1
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,5 @@
1-
// This file can be run or imported only if `--experimental-default-type=module` is set.
1+
// This file can be run or imported only if `--experimental-default-type=module` is set
2+
// or `--experimental-detect-module` is not disabled. If it's loaded by
3+
// require(), then `--experimental-require-module` must not be disabled.
24
export default 'module';
35
console.log('executed');

0 commit comments

Comments
 (0)