FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

esm: improve ERR_REQUIRE_ASYNC_MODULE · nodejs/node@851b460 · GitHub

/ node Public

Commit 851b460

Browse files
joyeecheung
authored and
aduh95
committed
esm: improve ERR_REQUIRE_ASYNC_MODULE
This brings back several improvements that were reverted by mistake when landing https://redirect.github.com/nodejs/node/pull/64154 - Update the documentation about how the removal of side effects of source collection - Add non-enumerable `requireStack` and `topLevelAwaitLocations` properties to `ERR_REQUIRE_ASYNC_MODULE`, the latter is only populated when --experimental-print-required-tla is enabled - Add "Required module: <url>" to the error message to identify the required ESM entry point regardless of whether the flag is enabled - Fix TLA caret column from 0-based to 1-based - Store module source via a private symbol instead of a public property - Use `hasAsyncGraph` (post-instantiation) in `throwIfAsyncGraph` instead of walking the graph before instantiation - Merge the require stack checking into the `common.expectRequiredTLAError` helper. - Removed tests that are made redundant by the snapshot tests Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: #64260 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
1 parent ea3b870 commit 851b460

24 files changed

Lines changed: 189 additions & 242 deletions

‎doc/api/errors.md‎

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2803,12 +2803,30 @@ A QUIC session failed because version negotiation is required.
28032803

28042804
### `ERR_REQUIRE_ASYNC_MODULE`
28052805

2806+
<!-- YAML
2807+
changes:
2808+
- version: REPLACEME
2809+
pr-url: https://github.com/nodejs/node/pull/64260
2810+
description: Added the `requireStack` and `topLevelAwaitLocations` properties.
2811+
-->
2812+
28062813
When trying to `require()` a [ES Module][], the module turns out to be asynchronous.
28072814
That is, it contains top-level await.
28082815

2809-
To see where the top-level await is, use
2810-
`--experimental-print-required-tla` (this would execute the modules
2811-
before looking for the top-level awaits).
2816+
When uncaught, the flag `--experimental-print-required-tla` prints
2817+
the locations of the top-level awaits in the graph to stderr.
2818+
2819+
This error has the following additional non-enumerable properties:
2820+
2821+
* `requireStack` {string\[]} The chain of modules that led to the failing
2822+
`require()`, starting with the module that required the asynchronous module.
2823+
* `topLevelAwaitLocations` {Object\[]} The locations of the top-level awaits in
2824+
the graph. Only populated when `--experimental-print-required-tla` is enabled.
2825+
Each entry has the following properties:
2826+
* `url` {string} The URL of the module containing the top-level await.
2827+
* `line` {number} The 1-based line number of the top-level await.
2828+
* `column` {number} The 1-based column number of the top-level await.
2829+
* `sourceLine` {string} The source line containing the top-level await.
28122830

28132831
<a id="ERR_REQUIRE_CYCLE_MODULE"></a>
28142832

‎doc/api/modules.md‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -316,10 +316,9 @@ graph it `import`s contains top-level `await`,
316316
[`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users should
317317
load the asynchronous module using [`import()`][].
318318

319-
If `--experimental-print-required-tla` is enabled, instead of throwing
320-
`ERR_REQUIRE_ASYNC_MODULE` before evaluation, Node.js will evaluate the
321-
module, try to locate the top-level awaits, and print their location to
322-
help users fix them.
319+
If `--experimental-print-required-tla` is enabled and the error is uncaught,
320+
Node.js will try to locate the top-level `await`s in the `require()`'d module graph
321+
and print the locations in the stderr.
323322

324323
If support for loading ES modules using `require()` results in unexpected
325324
breakage, it can be disabled using `--no-require-module`.

‎lib/internal/errors.js‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1714,20 +1714,36 @@ E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parent, locations) {
17141714
if (!getOptionValue('--experimental-print-required-tla')) {
17151715
message += ' To see where the top-level await comes from, use --experimental-print-required-tla.';
17161716
}
1717+
if (filename) {
1718+
message += `\nRequired module: ${filename}`;
1719+
}
17171720
if (parent) {
17181721
const { getRequireStack } = require('internal/modules/helpers');
17191722
const requireStack = getRequireStack(parent);
17201723
if (requireStack.length > 0) {
17211724
message += '\nRequire stack:\n- ' +
17221725
ArrayPrototypeJoin(requireStack, '\n- ');
17231726
}
1724-
this.requireStack = requireStack;
1727+
ObjectDefineProperty(this, 'requireStack', {
1728+
__proto__: null,
1729+
enumerable: false,
1730+
configurable: true,
1731+
writable: true,
1732+
value: requireStack,
1733+
});
17251734
}
17261735
if (locations && locations.length > 0) {
17271736
const { urlToFilename } = require('internal/modules/helpers');
17281737
const frames = ArrayPrototypeMap(locations, ({ url, line, column, sourceLine }) =>
1729-
`${urlToFilename(url)}:${line}\n\n${sourceLine}\n${StringPrototypeRepeat(' ', column)}^\n`);
1738+
`${urlToFilename(url)}:${line}\n\n${sourceLine}\n${StringPrototypeRepeat(' ', column - 1)}^\n`);
17301739
setArrowMessage(this, ArrayPrototypeJoin(frames, '\n'));
1740+
ObjectDefineProperty(this, 'topLevelAwaitLocations', {
1741+
__proto__: null,
1742+
enumerable: false,
1743+
configurable: true,
1744+
writable: true,
1745+
value: locations,
1746+
});
17311747
}
17321748
return message;
17331749
}, Error);

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,8 @@ class ModuleLoader {
291291
const status = job.module.getStatus();
292292
debug('Module status', job, status);
293293
// hasAsyncGraph is available after module been instantiated.
294-
if (status >= kInstantiated && job.module.hasAsyncGraph) {
295-
job.throwAsyncGraphError(parent);
294+
if (status >= kInstantiated) {
295+
job.throwIfAsyncGraph(parent);
296296
}
297297
if (status === kEvaluated) {
298298
return { wrap: job.module, namespace: job.module.getNamespace() };
@@ -320,9 +320,11 @@ class ModuleLoader {
320320
}
321321
if (status !== kEvaluating) {
322322
assert(status === kUninstantiated, `Unexpected module status ${status}`);
323-
// A previous require() of the same graph may have bailed out before
324-
// instantiation because it contains top-level await.
325-
job.throwIfAsyncGraph(parent);
323+
// If we get here, either there's a race where the job is still being instantiated
324+
// by an in-flight import(), or the cached module previously encountered an
325+
// instantiation error during a prior load (e.g. due to a mismatched import).
326+
// TODO(joyeecheung): the current check is too broad. We should attempt to
327+
// get the potential instantiation error and throw it.
326328
throw new ERR_REQUIRE_ESM_RACE_CONDITION(filename, parentFilename, false);
327329
}
328330
let message = `Cannot require() ES Module ${filename} in a cycle.`;

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

Lines changed: 61 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
const {
44
Array,
5+
ArrayIsArray,
56
ArrayPrototypeFind,
67
ArrayPrototypeJoin,
78
ArrayPrototypePop,
@@ -14,6 +15,7 @@ const {
1415
PromiseResolve,
1516
RegExpPrototypeExec,
1617
RegExpPrototypeSymbolReplace,
18+
RegExpPrototypeSymbolSplit,
1719
SafePromiseAllReturnArrayLike,
1820
SafePromiseAllReturnVoid,
1921
SafeSet,
@@ -37,8 +39,10 @@ const {
3739
kUninstantiated,
3840
} = internalBinding('module_wrap');
3941
const {
42+
getPromiseDetails,
4043
privateSymbols: {
4144
entry_point_module_private_symbol,
45+
module_source_private_symbol: kModuleSource,
4246
},
4347
} = internalBinding('util');
4448
/**
@@ -135,12 +139,12 @@ const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
135139
* @typedef {object} TopLevelAwaitLocation
136140
* @property {string} url URL of the module containing the top-level await.
137141
* @property {number} line 1-based line number of the top-level await.
138-
* @property {number} column 0-based column number of the top-level await.
142+
* @property {number} column 1-based column number of the top-level await.
139143
* @property {string} sourceLine The source line containing the top-level await.
140144
*/
141145

142146
/**
143-
* Locate the top-level awaits in the given module by parsing the source with acron.
147+
* Locate the top-level awaits in the given module by parsing the source with acorn.
144148
* @param {string} source Module source code.
145149
* @returns {object[]} The acorn AST nodes of the top-level awaits, in source order.
146150
*/
@@ -174,27 +178,62 @@ function findTopLevelAwait(source) {
174178
return found;
175179
}
176180

181+
/**
182+
* Collect the modules that contain top-level await in the linked graph of a job.
183+
* @param {ModuleJobBase} root The root of the module graph to search.
184+
* @returns {ModuleWrap[]} Modules that contain top-level await.
185+
*/
186+
function findModulesWithTopLevelAwait(root) {
187+
const found = [];
188+
const seen = new SafeSet();
189+
const stack = [root];
190+
while (stack.length > 0) {
191+
const job = ArrayPrototypePop(stack);
192+
if (seen.has(job)) { continue; }
193+
seen.add(job);
194+
if (job.module?.hasTopLevelAwait) {
195+
ArrayPrototypePush(found, job.module);
196+
}
197+
let linked = job.linked;
198+
if (isPromise(linked)) {
199+
linked = getPromiseDetails(linked)?.[1];
200+
}
201+
// If `require(esm)` comes from the deprecated async loader hook worker thread,
202+
// linked may be pending at this point. In that case, this branch would be skipped -
203+
// we just allow lossy reporting of TLA locations in an edge case when a deprecated
204+
// feature is used in combination with another experimental flag.
205+
if (ArrayIsArray(linked)) {
206+
for (let i = 0; i < linked.length; i++) {
207+
ArrayPrototypePush(stack, linked[i]);
208+
}
209+
}
210+
}
211+
return found;
212+
}
213+
177214
/**
178215
* Locate the top-level awaits in the given modules.
179-
* @param {ModuleWrap[]} modules Modules that may contain top-level await.
216+
* @param {ModuleJobBase} root The root of the module graph to search.
180217
* @returns {TopLevelAwaitLocation[]} The locations of the top-level awaits.
181218
*/
182-
function getTopLevelAwaitLocations(modules) {
219+
function getTopLevelAwaitLocations(root) {
220+
const modules = findModulesWithTopLevelAwait(root);
183221
const locations = [];
184222
for (let i = 0; i < modules.length; i++) {
185223
const module = modules[i];
186-
const source = module.source;
224+
const source = module[kModuleSource];
187225
if (typeof source !== 'string') { continue; } // Not retained during compilation. Skip.
188226
const found = findTopLevelAwait(source);
189227
if (found.length === 0) { continue; }
190-
const lines = StringPrototypeSplit(source, '\n');
228+
const lines = RegExpPrototypeSymbolSplit(/\r?\n/, source);
191229
for (let j = 0; j < found.length; j++) {
192230
const { start } = found[j].loc;
193231
ArrayPrototypePush(locations, {
194232
__proto__: null,
195233
url: module.url,
196234
line: start.line,
197-
column: start.column,
235+
// Acorn reports 0-based columns, convert them to 1-based to match `line`.
236+
column: start.column + 1,
198237
sourceLine: lines[start.line - 1],
199238
});
200239
}
@@ -261,61 +300,18 @@ class ModuleJobBase {
261300
}
262301

263302
/**
264-
* Collect the modules that contain top-level await in the linked graph of
265-
* this job. Whether each module contains top-level await is known at
266-
* compilation, so for a synchronously linked graph this finds asynchronous
267-
* graphs before instantiation.
268-
* On the (deprecated) async loader hook worker thread, linking may be asynchronous, in
269-
* which case the subgraphs that are not synchronously linked are skipped
270-
* and callers should still consult hasAsyncGraph after instantiation.
271-
* @returns {ModuleWrap[]}
272-
*/
273-
findModulesWithTopLevelAwait() {
274-
const found = [];
275-
const seen = new SafeSet();
276-
const stack = [this];
277-
while (stack.length > 0) {
278-
const job = ArrayPrototypePop(stack);
279-
if (seen.has(job)) { continue; }
280-
seen.add(job);
281-
if (job.module?.hasTopLevelAwait) {
282-
ArrayPrototypePush(found, job.module);
283-
}
284-
// job.linked is the array of evaluation-phase dependency jobs when the
285-
// linking is synchronous. Skip it if it's still a promise.
286-
if (!isPromise(job.linked)) {
287-
for (let i = 0; i < job.linked.length; i++) {
288-
ArrayPrototypePush(stack, job.linked[i]);
289-
}
290-
}
291-
}
292-
return found;
293-
}
294-
295-
/**
296-
* Throw the ERR_REQUIRE_ASYNC_MODULE with metadata for a require()'d graph that
297-
* contains top-level await.
298-
* @param {Module|undefined} parent CommonJS module that require()'d this, if any.
299-
* @param {ModuleWrap[]} [modules] Modules with top-level await, when already
300-
* collected by the caller, to avoid walking the graph again.
301-
*/
302-
throwAsyncGraphError(parent, modules = this.findModulesWithTopLevelAwait()) {
303-
const locations = getOptionValue('--experimental-print-required-tla') ? getTopLevelAwaitLocations(modules) : [];
304-
const filename = urlToFilename(this.url);
305-
throw new ERR_REQUIRE_ASYNC_MODULE(filename, parent, locations);
306-
}
307-
308-
/**
309-
* If the a require()'d graph contains top-level await, collect the source locations
303+
* If the require()'d graph contains top-level await, collect the source locations
310304
* of the top-level awaits using source code retained during compilation and throw
311-
* ERR_REQUIRE_ASYNC_MODULE. This can be run before instantiation is complete.
305+
* ERR_REQUIRE_ASYNC_MODULE. The module must be at least instantiated.
312306
* @param {Module|undefined} parent CommonJS module that require()'d this, if any.
313307
*/
314308
throwIfAsyncGraph(parent) {
315-
const modules = this.findModulesWithTopLevelAwait();
316-
if (modules.length > 0) {
317-
this.throwAsyncGraphError(parent, modules);
309+
if (!this.module.hasAsyncGraph) {
310+
return;
318311
}
312+
const locations = getOptionValue('--experimental-print-required-tla') ? getTopLevelAwaitLocations(this) : [];
313+
const filename = urlToFilename(this.url);
314+
throw new ERR_REQUIRE_ASYNC_MODULE(filename, parent, locations);
319315
}
320316

321317
/**
@@ -537,19 +533,15 @@ class ModuleJob extends ModuleJobBase {
537533
status = this.module.getStatus();
538534
}
539535
if (status === kInstantiated || status === kErrored) {
540-
if (this.module.hasAsyncGraph) {
541-
this.throwAsyncGraphError(parent);
542-
}
536+
this.throwIfAsyncGraph(parent);
543537
if (status === kInstantiated) {
544538
setHasStartedUserESMExecution();
545539
const namespace = this.module.evaluateSync();
546540
return { __proto__: null, module: this.module, namespace };
547541
}
548542
throw this.module.getError();
549543
} else if (status === kEvaluating || status === kEvaluated) {
550-
if (this.module.hasAsyncGraph) {
551-
this.throwAsyncGraphError(parent);
552-
}
544+
this.throwIfAsyncGraph(parent);
553545
// kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles.
554546
// Allow it for now, since we only need to ban ESM <-> CJS cycles which would be
555547
// detected earlier during the linking phase, though the CJS handling in the ESM
@@ -645,12 +637,11 @@ class ModuleJobSync extends ModuleJobBase {
645637
}
646638
return { __proto__: null, module: this.module };
647639
} else if (status === kInstantiated || status === kUninstantiated) {
648-
// The require() of this (synchronously linked) module bailed out: either
649-
// it was rejected for containing top-level await after instantiation
650-
// (kInstantiated), or its instantiation failed and left it uninstantiated
651-
// (kUninstantiated, e.g. a missing named export). When it's reached via async
652-
// run() from import, finish the instantiation and evaluate it asynchronously,
653-
// re-throwing any instantiation error.
640+
// If we get here, the module was initially required and is now being imported.
641+
// The require() module failed either because the graph has TLA (kInstantiated),
642+
// or instantiation failed (kUninstantiated, e.g. missing named export).
643+
// Try finishing the instantiation - if it succeeds, proceed to evaluation,
644+
// otherwise the branch below re-throw any instantiation error.
654645
if (status === kUninstantiated) {
655646
this.module.instantiate();
656647
}
@@ -676,9 +667,7 @@ class ModuleJobSync extends ModuleJobBase {
676667
// On the deprecated async loader hook worker thread, dependencies linked by an
677668
// earlier import may not be walkable synchronously, so double-check with
678669
// V8 now that the graph is instantiated.
679-
if (this.module.hasAsyncGraph) {
680-
this.throwAsyncGraphError(parent);
681-
}
670+
this.throwIfAsyncGraph(parent);
682671
setHasStartedUserESMExecution();
683672
try {
684673
const namespace = this.module.evaluateSync();

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ const {
1111
const {
1212
privateSymbols: {
1313
host_defined_option_symbol,
14+
module_source_private_symbol: kModuleSource,
1415
},
1516
} = internalBinding('util');
1617
const {
@@ -368,7 +369,7 @@ function compileSourceTextModule(url, source, type, context = kEmptyObject) {
368369
// only serves as a shortcut.
369370
if (wrap.hasTopLevelAwait &&
370371
getOptionValue('--experimental-print-required-tla')) {
371-
wrap.source = source;
372+
wrap[kModuleSource] = source;
372373
}
373374

374375
// Cache the source map for the module if present.

0 commit comments

Comments
 (0)
Back | FazBrowse Home | New Git URL