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 23 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
23 commits
Select commit Hold shift + click to select a range
c9531ef
module: improve error handling for top-level await in CommonJS
Nov 29, 2024
9dfd63d
repair
Nov 30, 2024
1b637d6
Update src/node_contextify.cc
mertcanaltin Nov 30, 2024
04e570f
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
1e12966
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
11d788e
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
2f8e7d4
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
bd58f77
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
2e77c7c
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
42387e9
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
34e2c3c
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
5040118
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
8e720b1
Update test/es-module/test-esm-detect-ambiguous.mjs
mertcanaltin Nov 30, 2024
ef625f3
improve error handling for top-level await in CommonJS
Dec 2, 2024
bf87e8d
test repair
Dec 2, 2024
f9d84b2
test repair
Dec 2, 2024
b1f54e3
added test for top-level
Dec 2, 2024
94f5a35
Update test-esm-detect-ambiguous.mjs
mertcanaltin Dec 3, 2024
9099572
fix: Improve top-level await handling and test reliability
Dec 10, 2024
b76fdb4
lint
Dec 10, 2024
c3a13fb
test repair
Dec 10, 2024
bb74388
test repair
Dec 10, 2024
01b4b6d
Refine error handling for top-level await in CommonJS modules
Dec 10, 2024
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
20 changes: 20 additions & 0 deletions src/node_contextify.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1816,6 +1816,26 @@ bool ShouldRetryAsESM(Realm* realm,
Utf8Value message_value(isolate, message);
auto message_view = message_value.ToStringView();

for (const auto& error_message : throws_only_in_cjs_error_messages) {
if (message_view.find(error_message) != std::string_view::npos &&
error_message.find("await is only valid in async functions and "
"the top level bodies of modules") !=
std::string_view::npos) {
const char* error_text =
"Top-level await is not supported in CommonJS modules. "
"To use top-level await, add \"type\": \"module\" to your "
"package.json "
"or rename the file to use the .mjs extension. Alternatively, wrap "
"the await expression in an async function. Module syntax like "
"import/export statements requires proper module configuration.";

isolate->ThrowException(v8::Exception::SyntaxError(
Copy link
Member

Choose a reason for hiding this comment

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

Throwing here would hide the original error, which makes it harder for users to locate e.g. where that top level await is.

String::NewFromUtf8(isolate, error_text).ToLocalChecked()));

return true;
}
}

// These indicates that the file contains syntaxes that are only valid in
// ESM. So it must be true.
for (const auto& error_message : esm_syntax_error_messages) {
Expand Down
36 changes: 26 additions & 10 deletions 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 @@ -246,9 +246,12 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
'await Promise.resolve(); console.log("executed");',
]);

strictEqual(stderr, '');
strictEqual(stdout, 'executed\n');
strictEqual(code, 0);
match(
stderr,
/Top-level await is not supported in CommonJS modules\. To use top-level await, add "type": "module" to your package\.json or rename the file to use the \.mjs extension/
Copy link
Member

Choose a reason for hiding this comment

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

This test is titled “permits top-level await”. This PR shouldn’t have the effect of prohibiting top-level await in ambiguous files that get detected to be ESM. This isn’t a test that should need updating as part of the goals of this PR.

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

Expand All @@ -258,9 +261,12 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
fixtures.path('es-modules/tla/unresolved.js'),
]);

strictEqual(stderr, '');
match(
stderr,
/SyntaxError: Top-level await is not supported in CommonJS modules\. To use top-level await, add "type": "module" to your package\.json or rename the file to use the \.mjs extension/
Copy link
Member

Choose a reason for hiding this comment

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

This test is titled “reports unfinished top-level await”. The code here should be parsed and run as ESM, and it should error with exit code 13 like the original test. This isn’t a test that should need updating as part of the goals of this PR.

);
strictEqual(stdout, '');
strictEqual(code, 13);
strictEqual(code, 1);
strictEqual(signal, null);
});

Expand All @@ -270,9 +276,12 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
'await Promise.resolve(); import "node:os"; console.log("executed");',
]);

strictEqual(stderr, '');
strictEqual(stdout, 'executed\n');
strictEqual(code, 0);
match(
stderr,
/Top-level await is not supported in CommonJS modules\. To use top-level await, add "type": "module" to your package\.json or rename the file to use the \.mjs extension/
Copy link
Member

Choose a reason for hiding this comment

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

This test is titled “permits top-level await above import/export syntax”. Based on the change you made to the assertion, your PR breaks this test.

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

Expand All @@ -282,7 +291,10 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
'function fn() { await Promise.resolve(); } fn();',
]);

match(stderr, /SyntaxError: await is only valid in async function/);
match(
stderr,
/Top-level await is not supported in CommonJS modules/
Copy link
Member

Choose a reason for hiding this comment

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

This test is titled “still throws on await in an ordinary sync function”. Based on the change you made to the assertion, your PR breaks this test.

);
strictEqual(stdout, '');
strictEqual(code, 1);
strictEqual(signal, null);
Expand All @@ -294,7 +306,11 @@ 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,
/SyntaxError: Top-level await is not supported in CommonJS modules\. To use top-level await, add "type": "module" to your package\.json or rename the file to use the \.mjs extension\. Alternatively, wrap the await expression in an async function\./
);
Copy link
Member

Choose a reason for hiding this comment

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

This test is titled “throws on undefined require when top-level await triggers ESM parsing”. That’s not happening anymore per your assertion here.

I think erroring about the top-level await instead of the undefined require is a better approach, but then the test title needs updating.


strictEqual(stdout, '');
strictEqual(code, 1);
strictEqual(signal, null);
Expand Down
Loading