Skip to content

module: improve error message for top-level await in CommonJS #55874

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.

Sign up for GitHub

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

module: improve error message for top-level await in CommonJS #55874

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
77c3d19
module: update tests for combined ambiguous module syntax error
mertcanaltin May 31, 2025
10e5620
lint
mertcanaltin May 31, 2025
b0d51ac
revert regex edit
mertcanaltin Jun 2, 2025
ab7ee6e
repair error message
mertcanaltin Jun 2, 2025
5725135
refactored ERR_AMBIGUOUS_MODULE_SYNTAX error
mertcanaltin Jun 3, 2025
f9bd373
repair hasTopLevelAwait & tests
mertcanaltin Jun 3, 2025
aab10e5
lint
mertcanaltin Jun 3, 2025
d756f7f
revert old change
mertcanaltin Jun 3, 2025
a2a9302
test repair
mertcanaltin Jun 3, 2025
53b03da
repair e.message
mertcanaltin Jun 3, 2025
32247d1
register HasTopLevelAwait method in cpp api
mertcanaltin Jun 3, 2025
811e9c5
lint
mertcanaltin Jun 3, 2025
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
13 changes: 10 additions & 3 deletions lib/internal/modules/esm/module_job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,9 +72,16 @@ const isCommonJSGlobalLikeNotDefinedError = (errorMessage) =>
* @param {string} url
* @returns {void}
*/
const explainCommonJSGlobalLikeNotDefinedError = (e, url) => {
const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
if (e?.name === 'ReferenceError' &&
isCommonJSGlobalLikeNotDefinedError(e.message)) {

if (hasTopLevelAwait) {
e.message = `ERR_AMBIGUOUS_MODULE_SYNTAX: This file cannot be parsed as either CommonJS or ES Module. CommonJS error: await is only valid in async functions. ES Module error: require is not defined in ES module scope. If you meant to use CommonJS, wrap top-level await in async function. If you meant to use ESM, do not use require().`;
Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'm a little confused here -- the code mostly looks good, but maybe we can spell out which condition exactly is supposed to trigger this error, and be specific about the error message? Like, it's odd to describe multiple different errors here, although I kind of see where you're coming from

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi @addaleax,

You're absolutely right - the message is trying to do too much.

The specific condition is: file has both require() and top-level await, creating an ambiguous parsing situation.

How about we simplify the error message to focus on the immediate issue:

ERR_AMBIGUOUS_MODULE_SYNTAX: Cannot use 'require()' and top-level 'await' in the same file.
Choose either CommonJS (remove await, use async functions) or ES Module (remove require, use import).

Copy link
Member

Choose a reason for hiding this comment

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

Technically it can be parsed as ESM, just not successfully evaluated as ESM because require is undefined. It can't be successfully parsed as CommonJS because of the top-level await. So I'm not sure if "ambiguous" is the right word; it's more that it's so unclear what the user intended that we can't reasonably make a guess.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically it can be parsed as ESM, just not successfully evaluated as ESM because require is undefined. It can't be successfully parsed as CommonJS because of the top-level await. So I'm not sure if "ambiguous" is the right word; it's more that it's so unclear what the user intended that we can't reasonably make a guess.

Yes, I agree with what you said, I think a message like this would be more appropriate.

Cannot determine intended module format. File contains both 'require()' and top-level 'await'. For CommonJS: wrap await in async function. For ES Module: replace require() with import.

e.code = 'ERR_AMBIGUOUS_MODULE_SYNTAX';
return;
}

e.message += ' in ES module scope';

if (StringPrototypeStartsWith(e.message, 'require ')) {
Expand Down Expand Up @@ -357,7 +364,7 @@ class ModuleJob extends ModuleJobBase {
try {
await this.module.evaluate(timeout, breakOnSigint);
} catch (e) {
explainCommonJSGlobalLikeNotDefinedError(e, this.module.url);
explainCommonJSGlobalLikeNotDefinedError(e, this.module.url, this.module.hasTopLevelAwait());
throw e;
}
return { __proto__: null, module: this.module };
Expand Down Expand Up @@ -491,7 +498,7 @@ class ModuleJobSync extends ModuleJobBase {
const namespace = this.module.evaluateSync(filename, parentFilename);
return { __proto__: null, module: this.module, namespace };
} catch (e) {
explainCommonJSGlobalLikeNotDefinedError(e, this.module.url);
explainCommonJSGlobalLikeNotDefinedError(e, this.module.url, this.module.hasTopLevelAwait());
throw e;
}
}
Expand Down
24 changes: 24 additions & 0 deletions src/module_wrap.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -877,6 +877,27 @@ void ModuleWrap::IsGraphAsync(const FunctionCallbackInfo<Value>& args) {
args.GetReturnValue().Set(module->IsGraphAsync());
}

void ModuleWrap::HasTopLevelAwait(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
ModuleWrap* obj;
ASSIGN_OR_RETURN_UNWRAP(&obj, args.This());

Local<Module> module = obj->module_.Get(isolate);

// Check if module is valid
if (module.IsEmpty()) {
args.GetReturnValue().Set(false);
return;
}

// For source text modules, check if the graph is async
// For synthetic modules, it's always false
bool has_top_level_await =
module->IsSourceTextModule() && module->IsGraphAsync();

args.GetReturnValue().Set(has_top_level_await);
}

void ModuleWrap::GetError(const FunctionCallbackInfo<Value>& args) {
Isolate* isolate = args.GetIsolate();
ModuleWrap* obj;
Expand Down Expand Up @@ -1305,6 +1326,8 @@ void ModuleWrap::CreatePerIsolateProperties(IsolateData* isolate_data,
SetProtoMethodNoSideEffect(isolate, tpl, "getNamespace", GetNamespace);
SetProtoMethodNoSideEffect(isolate, tpl, "getStatus", GetStatus);
SetProtoMethodNoSideEffect(isolate, tpl, "isGraphAsync", IsGraphAsync);
SetProtoMethodNoSideEffect(
isolate, tpl, "hasTopLevelAwait", HasTopLevelAwait);
SetProtoMethodNoSideEffect(isolate, tpl, "getError", GetError);
SetConstructorFunction(isolate, target, "ModuleWrap", tpl);
isolate_data->set_module_wrap_constructor_template(tpl);
Expand Down Expand Up @@ -1367,6 +1390,7 @@ void ModuleWrap::RegisterExternalReferences(
registry->Register(GetStatus);
registry->Register(GetError);
registry->Register(IsGraphAsync);
registry->Register(HasTopLevelAwait);

registry->Register(CreateRequiredModuleFacade);

Expand Down
1 change: 1 addition & 0 deletions src/module_wrap.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ class ModuleWrap : public BaseObject {
void MemoryInfo(MemoryTracker* tracker) const override {
tracker->TrackField("resolve_cache", resolve_cache_);
}
static void HasTopLevelAwait(const v8::FunctionCallbackInfo<v8::Value>& args);

v8::Local<v8::Context> context() const;
v8::Maybe<bool> CheckUnsettledTopLevelAwait();
Expand Down
26 changes: 25 additions & 1 deletion test/es-module/test-esm-detect-ambiguous.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,10 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
'const fs = require("node:fs"); await Promise.resolve();',
]);

match(stderr, /ReferenceError: require is not defined in ES module scope/);
match(
stderr,
/ERR_AMBIGUOUS_MODULE_SYNTAX: This file cannot be parsed as either CommonJS or ES Module\. CommonJS error: await is only valid in async functions\. ES Module error: require is not defined in ES module scope\. If you meant to use CommonJS, wrap top-level await in async function\. If you meant to use ESM, do not use require\(\)\./
);
strictEqual(stdout, '');
strictEqual(code, 1);
strictEqual(signal, null);
Expand Down Expand Up @@ -423,3 +426,24 @@ describe('when working with Worker threads', () => {
strictEqual(signal, null);
});
});

describe('cjs & esm ambiguous syntax case', () => {
it('should throw an ambiguous syntax error when using top-level await with require', async () => {
const { stderr, code, signal } = await spawnPromisified(
process.execPath,
[
'--input-type=module',
'--eval',
`await 1;\nconst fs = require('fs');`,
]
);

match(
stderr,
/ERR_AMBIGUOUS_MODULE_SYNTAX: This file cannot be parsed as either CommonJS or ES Module\. CommonJS error: await is only valid in async functions\. ES Module error: require is not defined in ES module scope\. If you meant to use CommonJS, wrap top-level await in async function\. If you meant to use ESM, do not use require\(\)\./
);

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