Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
29 changes: 20 additions & 9 deletions lib/internal/modules/esm/module_job.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
const {
Array,
ArrayPrototypeJoin,
ArrayPrototypeSome,
FunctionPrototype,
ObjectSetPrototypeOf,
PromisePrototypeThen,
Expand Down Expand Up @@ -61,11 +60,14 @@ const CJSGlobalLike = [
'__filename',
'__dirname',
];
const isCommonJSGlobalLikeNotDefinedError = (errorMessage) =>
ArrayPrototypeSome(
CJSGlobalLike,
(globalLike) => errorMessage === `${globalLike} is not defined`,
);
const getUndefinedCJSGlobalLike = (errorMessage) => {
for (const globalLike of CJSGlobalLike) {
if (errorMessage === `${globalLike} is not defined`) {
return globalLike;
}
}
return null;
};


/**
Expand All @@ -75,11 +77,20 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) =>
* @returns {void}
*/
const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
if (e?.name === 'ReferenceError' &&
isCommonJSGlobalLikeNotDefinedError(e.message)) {
const undefinedGlobal = getUndefinedCJSGlobalLike(e?.message);
if (e?.name === 'ReferenceError' && undefinedGlobal !== null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const undefinedGlobal = getUndefinedCJSGlobalLike(e?.message);
if (e?.name === 'ReferenceError' && undefinedGlobal !== null) {
const notDefinedGlobalLike = e?.name === 'ReferenceError' && findCommonJSGlobalLikeNotDefinedError(e.message);
if (notDefinedGlobalLike) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wdyt of this suggestion? That would remove one optional chaining, and skip the function call for unrelated errorrs

Copy link
Contributor Author

@mag123c mag123c Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just my codding style. 😢 updated to use short-circuit evaluation. Thanks!


if (hasTopLevelAwait) {
e.message = `Cannot determine intended module format because both require() and top-level await are present. If the code is intended to be CommonJS, wrap await in an async function. If the code is intended to be an ES module, replace require() with import.`;
let advice = '';
if (undefinedGlobal === 'require') {
advice = 'replace require() with import';
} else if (undefinedGlobal === 'module' || undefinedGlobal === 'exports') {
advice = 'use export instead of module.exports/exports';
} else if (undefinedGlobal === '__filename' || undefinedGlobal === '__dirname') {
advice = 'use import.meta.url instead';
}

e.message = `Cannot determine intended module format because both ${undefinedGlobal} and top-level await are present. If the code is intended to be CommonJS, wrap await in an async function. If the code is intended to be an ES module, ${advice}.`;
e.code = 'ERR_AMBIGUOUS_MODULE_SYNTAX';
return;
}
Expand Down
25 changes: 21 additions & 4 deletions test/es-module/test-esm-detect-ambiguous.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -283,7 +283,7 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },

match(
stderr,
/ReferenceError: Cannot determine intended module format because both require\(\) and top-level await are present\. If the code is intended to be CommonJS, wrap await in an async function\. If the code is intended to be an ES module, replace require\(\) with import\./
/ReferenceError: Cannot determine intended module format because both require and top-level await are present\. If the code is intended to be CommonJS, wrap await in an async function\. If the code is intended to be an ES module, replace require\(\) with import\./
);
strictEqual(stdout, '');
strictEqual(code, 1);
Expand Down Expand Up @@ -432,15 +432,32 @@ describe('cjs & esm ambiguous syntax case', () => {
const { stderr, code, signal } = await spawnPromisified(
process.execPath,
[
'--input-type=module',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed --input-type=module because I saw that line 278 already tests the same error without the flag:

const fs = require("node:fs"); await Promise.resolve();

I thought having both tests (with and without --input-type=module) was redundant since they produce the same error message. However, if there's a specific reason to test the explicit module type specification scenario separately, I can revert this change.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've revert changes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you have and you forgot to push your changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's already there. I had removed it in an earlier commit, but reverted it back in 9457af9.

'--eval',
`await 1;\nconst fs = require('fs');`,
`const fs = require('fs');\nawait 1;`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the order to match the pattern in the existing test at line 278 (const fs = require("node:fs"); await Promise.resolve();). However, I realize this might be an unnecessary change if the original order was intentional. Should I revert this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think it should be reverted, both order should be supported so it makes sense to test both IMO

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've revert changes.

]
);

match(
stderr,
/ReferenceError: Cannot determine intended module format because both require\(\) and top-level await are present\. If the code is intended to be CommonJS, wrap await in an async function\. If the code is intended to be an ES module, replace require\(\) with import\./
/ReferenceError: Cannot determine intended module format because both require and top-level await are present\. If the code is intended to be CommonJS, wrap await in an async function\. If the code is intended to be an ES module, replace require\(\) with import\./
);

strictEqual(code, 1);
strictEqual(signal, null);
});

it('should throw an ambiguous syntax error when using top-level await with exports', async () => {
const { stderr, code, signal } = await spawnPromisified(
process.execPath,
[
'--eval',
`exports.foo = 'bar';\nawait 1;`,
]
);

match(
stderr,
/ReferenceError: Cannot determine intended module format because both exports and top-level await are present\. If the code is intended to be CommonJS, wrap await in an async function\. If the code is intended to be an ES module, use export instead of module\.exports\/exports\./
);

strictEqual(code, 1);
Expand Down