Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

module: check --experimental-require-module separately from detection #55250

Merged
merged 1 commit into from
Oct 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 12 additions & 4 deletions lib/internal/modules/cjs/loader.js
Original file line number Diff line number Diff line change
Expand Up @@ -1446,7 +1446,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) {
Expand Down Expand Up @@ -1476,7 +1476,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.
Expand Down Expand Up @@ -1506,8 +1516,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;
Expand Down
19 changes: 19 additions & 0 deletions test/es-module/test-disable-require-module-with-detection.js
Original file line number Diff line number Diff line change
@@ -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'/
});
4 changes: 3 additions & 1 deletion test/fixtures/es-modules/loose.js
Original file line number Diff line number Diff line change
@@ -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');
Loading