Skip to content

Commit ba7f8a0

Browse files
joyeecheungmarco-ippolito
joyeecheung
authored and
marco-ippolito
committed
module: improve error message from asynchronicity in require(esm)
- Improve the error message that shows up when there is a race from doing require(esm) and import(esm) at the same time. - Improve error message of ERR_REQUIRE_ASYNC_MODULE by showing parent and target file names, if available. Drive-by: split the require(tla) tests since we are modifying the tests already. PR-URL: #57126 Backport-PR-URL: #59504 Refs: https://github.com/fisker/prettier-issue-17139 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Chengzhong Wu <[email protected]> Refs: #52697
1 parent eb84204 commit ba7f8a0

13 files changed

+190
-79
lines changed

‎lib/internal/errors.js‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1692,9 +1692,18 @@ E('ERR_PARSE_ARGS_UNKNOWN_OPTION', (option, allowPositionals) => {
16921692
E('ERR_PERFORMANCE_INVALID_TIMESTAMP',
16931693
'%d is not a valid timestamp', TypeError);
16941694
E('ERR_PERFORMANCE_MEASURE_INVALID_OPTIONS', '%s', TypeError);
1695-
E('ERR_REQUIRE_ASYNC_MODULE', 'require() cannot be used on an ESM ' +
1696-
'graph with top-level await. Use import() instead. To see where the' +
1697-
' top-level await comes from, use --experimental-print-required-tla.', Error);
1695+
E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parentFilename) {
1696+
let message = 'require() cannot be used on an ESM ' +
1697+
'graph with top-level await. Use import() instead. To see where the' +
1698+
' top-level await comes from, use --experimental-print-required-tla.';
1699+
if (parentFilename) {
1700+
message += `\n From ${parentFilename} `;
1701+
}
1702+
if (filename) {
1703+
message += `\n Requiring ${filename} `;
1704+
}
1705+
return message;
1706+
}, Error);
16981707
E('ERR_REQUIRE_CYCLE_MODULE', '%s', Error);
16991708
E('ERR_REQUIRE_ESM',
17001709
function(filename, hasEsmSyntax, parentPath = null, packageJsonPath = null) {

‎lib/internal/modules/esm/loader.js‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -295,20 +295,37 @@ class ModuleLoader {
295295
// TODO(joyeecheung): ensure that imported synchronous graphs are evaluated
296296
// synchronously so that any previously imported synchronous graph is already
297297
// evaluated at this point.
298+
// TODO(joyeecheung): add something similar to CJS loader's requireStack to help
299+
// debugging the the problematic links in the graph for import.
298300
if (job !== undefined) {
299301
mod[kRequiredModuleSymbol] = job.module;
302+
const parentFilename = urlToFilename(parent?.filename);
303+
// TODO(node:55782): this race may stop to happen when the ESM resolution and loading become synchronous.
304+
if (!job.module) {
305+
let message = `Cannot require() ES Module ${filename} because it is not yet fully loaded. `;
306+
message += 'This may be caused by a race condition if the module is simultaneously dynamically ';
307+
message += 'import()-ed via Promise.all(). Try await-ing the import() sequentially in a loop instead.';
308+
if (parentFilename) {
309+
message += ` (from ${parentFilename})`;
310+
}
311+
assert(job.module, message);
312+
}
300313
if (job.module.async) {
301-
throw new ERR_REQUIRE_ASYNC_MODULE();
314+
throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename);
302315
}
316+
// job.module may be undefined if it's asynchronously loaded. Which means
317+
// there is likely a cycle.
303318
if (job.module.getStatus() !== kEvaluated) {
304-
const parentFilename = urlToFilename(parent?.filename);
305319
let message = `Cannot require() ES Module ${filename} in a cycle.`;
306320
if (parentFilename) {
307321
message += ` (from ${parentFilename})`;
308322
}
323+
message += 'A cycle involving require(esm) is disallowed to maintain ';
324+
message += 'invariants madated by the ECMAScript specification';
325+
message += 'Try making at least part of the dependency in the graph lazily loaded.';
309326
throw new ERR_REQUIRE_CYCLE_MODULE(message);
310327
}
311-
return { wrap: job.module, namespace: job.module.getNamespaceSync() };
328+
return { wrap: job.module, namespace: job.module.getNamespaceSync(filename, parentFilename) };
312329
}
313330
// TODO(joyeecheung): refactor this so that we pre-parse in C++ and hit the
314331
// cache here, or use a carrier object to carry the compiled module script
@@ -320,7 +337,7 @@ class ModuleLoader {
320337
job = new ModuleJobSync(this, url, kEmptyObject, wrap, isMain, inspectBrk);
321338
this.loadCache.set(url, kImplicitAssertType, job);
322339
mod[kRequiredModuleSymbol] = job.module;
323-
return { wrap: job.module, namespace: job.runSync().namespace };
340+
return { wrap: job.module, namespace: job.runSync(parent).namespace };
324341
}
325342

326343
/**

‎lib/internal/modules/esm/module_job.js‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ const assert = require('internal/assert');
3131
const resolvedPromise = PromiseResolve();
3232
const {
3333
setHasStartedUserESMExecution,
34+
urlToFilename,
3435
} = require('internal/modules/helpers');
3536
const { getOptionValue } = require('internal/options');
3637
const noop = FunctionPrototype;
@@ -371,7 +372,7 @@ class ModuleJobSync extends ModuleJobBase {
371372
`Status = ${status}`);
372373
}
373374

374-
runSync() {
375+
runSync(parent) {
375376
// TODO(joyeecheung): add the error decoration logic from the async instantiate.
376377
this.module.async = this.module.instantiateSync();
377378
// If --experimental-print-required-tla is true, proceeds to evaluation even
@@ -380,11 +381,13 @@ class ModuleJobSync extends ModuleJobBase {
380381
// TODO(joyeecheung): track the asynchroniticy using v8::Module::HasTopLevelAwait()
381382
// and we'll be able to throw right after compilation of the modules, using acron
382383
// to find and print the TLA.
384+
const parentFilename = urlToFilename(parent?.filename);
385+
const filename = urlToFilename(this.url);
383386
if (this.module.async && !getOptionValue('--experimental-print-required-tla')) {
384-
throw new ERR_REQUIRE_ASYNC_MODULE();
387+
throw new ERR_REQUIRE_ASYNC_MODULE(filename, parentFilename);
385388
}
386389
setHasStartedUserESMExecution();
387-
const namespace = this.module.evaluateSync();
390+
const namespace = this.module.evaluateSync(filename, parentFilename);
388391
return { __proto__: null, module: this.module, namespace };
389392
}
390393
}

‎src/module_wrap.cc‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,7 +648,7 @@ void ModuleWrap::EvaluateSync(const FunctionCallbackInfo<Value>& args) {
648648
FPrintF(stderr, "%s\n", reason);
649649
}
650650
}
651-
THROW_ERR_REQUIRE_ASYNC_MODULE(env);
651+
THROW_ERR_REQUIRE_ASYNC_MODULE(env, args[0], args[1]);
652652
return;
653653
}
654654

@@ -678,7 +678,7 @@ void ModuleWrap::GetNamespaceSync(const FunctionCallbackInfo<Value>& args) {
678678
}
679679

680680
if (module->IsGraphAsync()) {
681-
return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env());
681+
return THROW_ERR_REQUIRE_ASYNC_MODULE(realm->env(), args[0], args[1]);
682682
}
683683
Local<Value> result = module->GetModuleNamespace();
684684
args.GetReturnValue().Set(result);

‎src/node_errors.h‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -208,10 +208,6 @@ ERRORS_WITH_CODE(V)
208208
"creating Workers") \
209209
V(ERR_NON_CONTEXT_AWARE_DISABLED, \
210210
"Loading non context-aware native addons has been disabled") \
211-
V(ERR_REQUIRE_ASYNC_MODULE, \
212-
"require() cannot be used on an ESM graph with top-level await. Use " \
213-
"import() instead. To see where the top-level await comes from, use " \
214-
"--experimental-print-required-tla.") \
215211
V(ERR_SCRIPT_EXECUTION_INTERRUPTED, \
216212
"Script execution was interrupted by `SIGINT`") \
217213
V(ERR_TLS_PSK_SET_IDENTIY_HINT_FAILED, "Failed to set PSK identity hint") \
@@ -241,6 +237,28 @@ inline void THROW_ERR_SCRIPT_EXECUTION_TIMEOUT(Environment* env,
241237
THROW_ERR_SCRIPT_EXECUTION_TIMEOUT(env, message.str().c_str());
242238
}
243239

240+
inline void THROW_ERR_REQUIRE_ASYNC_MODULE(
241+
Environment* env,
242+
v8::Local<v8::Value> filename,
243+
v8::Local<v8::Value> parent_filename) {
244+
static constexpr const char* prefix =
245+
"require() cannot be used on an ESM graph with top-level await. Use "
246+
"import() instead. To see where the top-level await comes from, use "
247+
"--experimental-print-required-tla.";
248+
std::string message = prefix;
249+
if (!parent_filename.IsEmpty() && parent_filename->IsString()) {
250+
Utf8Value utf8(env->isolate(), parent_filename);
251+
message += "\n From ";
252+
message += utf8.out();
253+
}
254+
if (!filename.IsEmpty() && filename->IsString()) {
255+
Utf8Value utf8(env->isolate(), filename);
256+
message += "\n Requiring ";
257+
message += +utf8.out();
258+
}
259+
THROW_ERR_REQUIRE_ASYNC_MODULE(env, message.c_str());
260+
}
261+
244262
inline v8::Local<v8::Object> ERR_BUFFER_TOO_LARGE(v8::Isolate* isolate) {
245263
char message[128];
246264
snprintf(message, sizeof(message),

‎test/common/index.js‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -937,6 +937,17 @@ function expectRequiredModule(mod, expectation, checkESModule = true) {
937937
assert.deepStrictEqual(clone, { ...expectation });
938938
}
939939

940+
function expectRequiredTLAError(err) {
941+
const message = /require\(\) cannot be used on an ESM graph with top-level await/;
942+
if (typeof err === 'string') {
943+
assert.match(err, /ERR_REQUIRE_ASYNC_MODULE/);
944+
assert.match(err, message);
945+
} else {
946+
assert.strictEqual(err.code, 'ERR_REQUIRE_ASYNC_MODULE');
947+
assert.match(err.message, message);
948+
}
949+
}
950+
940951
const common = {
941952
allowGlobals,
942953
buildType,
@@ -946,6 +957,7 @@ const common = {
946957
defaultAutoSelectFamilyAttemptTimeout,
947958
expectsError,
948959
expectRequiredModule,
960+
expectRequiredTLAError,
949961
expectWarning,
950962
getArrayBufferViews,
951963
getBufferSources,

‎test/es-module/test-require-module-tla-execution.js‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
'use strict';
2+
3+
// Tests that require(esm) with top-level-await throws before execution starts
4+
// if --experimental-print-required-tla is not enabled.
5+
6+
const common = require('../common');
7+
const assert = require('assert');
8+
const { spawnSyncAndExit } = require('../common/child_process');
9+
const fixtures = require('../common/fixtures');
10+
11+
{
12+
spawnSyncAndExit(process.execPath, [
13+
fixtures.path('es-modules/tla/require-execution.js'),
14+
], {
15+
signal: null,
16+
status: 1,
17+
stderr(output) {
18+
assert.doesNotMatch(output, /I am executed/);
19+
common.expectRequiredTLAError(output);
20+
assert.match(output, /From .*require-execution\.js/);
21+
assert.match(output, /Requiring .*execution\.mjs/);
22+
return true;
23+
},
24+
stdout: ''
25+
});
26+
}

‎test/es-module/test-require-module-tla-nested.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
'use strict';
2+
3+
// Tests that require(esm) throws for top-level-await in inner graphs.
4+
5+
const common = require('../common');
6+
const assert = require('assert');
7+
8+
assert.throws(() => {
9+
require('../fixtures/es-modules/tla/parent.mjs');
10+
}, (err) => {
11+
common.expectRequiredTLAError(err);
12+
assert.match(err.message, /From .*test-require-module-tla-nested\.js/);
13+
assert.match(err.message, /Requiring .*parent\.mjs/);
14+
return true;
15+
});

‎test/es-module/test-require-module-tla-print-execution.js‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
'use strict';
2+
3+
// Tests that require(esm) with top-level-await throws after execution
4+
// if --experimental-print-required-tla is enabled.
5+
6+
const common = require('../common');
7+
const assert = require('assert');
8+
const { spawnSyncAndExit } = require('../common/child_process');
9+
const fixtures = require('../common/fixtures');
10+
11+
{
12+
spawnSyncAndExit(process.execPath, [
13+
'--experimental-print-required-tla',
14+
fixtures.path('es-modules/tla/require-execution.js'),
15+
], {
16+
signal: null,
17+
status: 1,
18+
stderr(output) {
19+
assert.match(output, /I am executed/);
20+
common.expectRequiredTLAError(output);
21+
assert.match(output, /Error: unexpected top-level await at.*execution\.mjs:3/);
22+
assert.match(output, /await Promise\.resolve\('hi'\)/);
23+
assert.match(output, /From .*require-execution\.js/);
24+
assert.match(output, /Requiring .*execution\.mjs/);
25+
return true;
26+
},
27+
stdout: ''
28+
});
29+
}

‎test/es-module/test-require-module-tla-rejected.js‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,15 @@
1+
'use strict';
2+
3+
// Tests that require(esm) throws for rejected top-level await.
4+
5+
const common = require('../common');
6+
const assert = require('assert');
7+
8+
assert.throws(() => {
9+
require('../fixtures/es-modules/tla/rejected.mjs');
10+
}, (err) => {
11+
common.expectRequiredTLAError(err);
12+
assert.match(err.message, /From .*test-require-module-tla-rejected\.js/);
13+
assert.match(err.message, /Requiring .*rejected\.mjs/);
14+
return true;
15+
});

0 commit comments

Comments
 (0)