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

Merged
nodejs-github-bot merged 14 commits into from
Jun 24, 2025
Merged

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

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
14 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
0f126d0
update error message expectation for module syntax
mertcanaltin Jun 24, 2025
ad40103
lint
mertcanaltin Jun 24, 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().`;

@addaleax addaleax Jun 5, 2025

Copy link
Copy Markdown
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

@mertcanaltin mertcanaltin Jun 5, 2025

Copy link
Copy Markdown
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).

@GeoffreyBooth GeoffreyBooth Jun 6, 2025

Copy link
Copy Markdown
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.

@mertcanaltin mertcanaltin Jun 6, 2025

Copy link
Copy Markdown
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.

@GeoffreyBooth GeoffreyBooth Jun 22, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I assume when you write For CommonJS: what you mean is, If you intended this file to be CommonJS, and likewise for For ES Module:. So maybe:

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.

@mertcanaltin mertcanaltin Jun 24, 2025

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Thanks I updated

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\(\)\./
);
Comment thread
mertcanaltin marked this conversation as resolved.
Resolved
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);
});
});