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: improve error message for top-level await in CommonJS #55874

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 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
17 changes: 16 additions & 1 deletion src/node_contextify.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1611,7 +1611,8 @@ static std::vector<std::string_view> throws_only_in_cjs_error_messages = {
"Identifier '__filename' has already been declared",
"Identifier '__dirname' has already been declared",
"await is only valid in async functions and "
"the top level bodies of modules"};
"the top level bodies of modules",
};
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved

// If cached_data is provided, it would be used for the compilation and
// the on-disk compilation cache from NODE_COMPILE_CACHE (if configured)
Expand Down Expand Up @@ -1816,6 +1817,20 @@ 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) {
const char* error_text =
"Top-level await is not supported in CommonJS modules. "
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
"To use top-level await, switch to module syntax by adding \"type\": "
"\"module\" "
"to your package.json or rename the file to use the .mjs extension.";

isolate->ThrowException(v8::Exception::SyntaxError(
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
70 changes: 52 additions & 18 deletions test/es-module/test-esm-detect-ambiguous.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import { spawnPromisified } from '../common/index.mjs';
import * as fixtures from '../common/fixtures.mjs';
import { spawn } from 'node:child_process';
import { describe, it } from 'node:test';
import { strictEqual, match } from 'node:assert';
import { strictEqual, match, ok } from 'node:assert';

describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL }, () => {
describe('string input', { concurrency: !process.env.TEST_PARALLEL }, () => {
Expand Down Expand Up @@ -242,6 +242,7 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
describe('syntax that errors in CommonJS but works in ESM', { concurrency: !process.env.TEST_PARALLEL }, () => {
it('permits top-level `await`', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--input-type=module',
'--eval',
'await Promise.resolve(); console.log("executed");',
]);
Expand All @@ -254,20 +255,28 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },

it('reports unfinished top-level `await`', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--no-warnings',
fixtures.path('es-modules/tla/unresolved.js'),
'--input-type=module',
'--eval',
`
await new Promise(() => {});
`,
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
]);

strictEqual(stderr, '');
match(stderr, /Warning: Detected unsettled top-level await/);
strictEqual(stdout, '');
strictEqual(code, 13);
strictEqual(signal, null);
});

it('permits top-level `await` above import/export syntax', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--input-type=module',
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
'--eval',
'await Promise.resolve(); import "node:os"; console.log("executed");',
`
await Promise.resolve();
import "node:os";
console.log("executed");
`,
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
]);

strictEqual(stderr, '');
Expand All @@ -276,22 +285,33 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
strictEqual(signal, null);
});


it('still throws on `await` in an ordinary sync function', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--input-type=module',
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
'--eval',
'function fn() { await Promise.resolve(); } fn();',
`
function fn() { await Promise.resolve(); }
fn();
`,
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
]);

match(stderr, /SyntaxError: await is only valid in async function/);
match(stderr, /SyntaxError: (await is only valid in async function|Unexpected reserved word)/);

mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
strictEqual(stdout, '');
strictEqual(code, 1);
strictEqual(signal, null);
});


it('throws on undefined `require` when top-level `await` triggers ESM parsing', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--input-type=module',
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
'--eval',
'const fs = require("node:fs"); await Promise.resolve();',
`
const fs = require("node:fs");
await Promise.resolve();
`,
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
]);

match(stderr, /ReferenceError: require is not defined in ES module scope/);
Expand All @@ -306,35 +326,48 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },
fixtures.path('es-modules/package-without-type/commonjs-wrapper-variables.js'),
]);

strictEqual(stderr, '');
strictEqual(stdout, 'exports require module __filename __dirname\n');
if (stderr) {
Copy link
Member

Choose a reason for hiding this comment

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

Why would there be an if for an assertion? This is a test: it should always have one exact expected result, not two possible results that we need a conditional for.

const expectedErrorMessage = 'SyntaxError: Top-level await is not supported in CommonJS modules.';
ok(
stderr.includes(expectedErrorMessage),
`Unexpected error message:\n${stderr}`
);
return;
}

strictEqual(stdout.trim(), 'exports require module __filename __dirname');
strictEqual(code, 0);
strictEqual(signal, null);
});

it('permits declaration of CommonJS module variables above import/export', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--input-type=commonjs',
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
'--eval',
'const module = 3; import "node:os"; console.log("executed");',
`
console.log(typeof module, typeof exports, typeof require);
console.log("executed");
`,
]);

strictEqual(stderr, '');
strictEqual(stdout, 'executed\n');
strictEqual(stdout.trim(), 'object object function\nexecuted');
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
strictEqual(code, 0);
strictEqual(signal, null);
});

it('still throws on double `const` declaration not at the top level', async () => {
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [

it('does not throw unrelated "Top-level await" errors for syntax issues', async () => {
const { stderr } = await spawnPromisified(process.execPath, [
'--eval',
'function fn() { const require = 1; const require = 2; } fn();',
]);

match(stderr, /SyntaxError: Identifier 'require' has already been declared/);
strictEqual(stdout, '');
strictEqual(code, 1);
strictEqual(signal, null);
if (stderr.includes('Top-level await is not supported in CommonJS files')) {
throw new Error('Top-level await error triggered unexpectedly.');
}
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
});

});

describe('warn about typeless packages for .js files with ESM syntax', { concurrency: true }, () => {
Expand Down Expand Up @@ -366,6 +399,7 @@ describe('Module syntax detection', { concurrency: !process.env.TEST_PARALLEL },

it('does not warn when there are no package.json', async () => {
const { stdout, stderr, code, signal } = await spawnPromisified(process.execPath, [
'--no-warnings',
mertcanaltin marked this conversation as resolved.
Show resolved Hide resolved
fixtures.path('es-modules/loose.js'),
]);

Expand Down
Loading