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

esm: improve ERR_REQUIRE_ASYNC_MODULE · nodejs/node@9e39360 · GitHub

/ node Public

Commit 9e39360

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 Backport-PR-URL: #65125 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
1 parent d2b02e4 commit 9e39360

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
@@ -2765,12 +2765,30 @@ A QUIC session failed because version negotiation is required.
27652765

27662766
### `ERR_REQUIRE_ASYNC_MODULE`
27672767

2768+
<!-- YAML
2769+
changes:
2770+
- version: REPLACEME
2771+
pr-url: https://github.com/nodejs/node/pull/64260
2772+
description: Added the `requireStack` and `topLevelAwaitLocations` properties.
2773+
-->
2774+
27682775
When trying to `require()` an [ES Module][], the module turns out to be asynchronous.
27692776
That is, it contains top-level await.
27702777

2771-
To see where the top-level await is, use
2772-
`--experimental-print-required-tla` (this would execute the modules
2773-
before looking for the top-level awaits).
2778+
When uncaught, the flag `--experimental-print-required-tla` prints
2779+
the locations of the top-level awaits in the graph to stderr.
2780+
2781+
This error has the following additional non-enumerable properties:
2782+
2783+
* `requireStack` {string\[]} The chain of modules that led to the failing
2784+
`require()`, starting with the module that required the asynchronous module.
2785+
* `topLevelAwaitLocations` {Object\[]} The locations of the top-level awaits in
2786+
the graph. Only populated when `--experimental-print-required-tla` is enabled.
2787+
Each entry has the following properties:
2788+
* `url` {string} The URL of the module containing the top-level await.
2789+
* `line` {number} The 1-based line number of the top-level await.
2790+
* `column` {number} The 1-based column number of the top-level await.
2791+
* `sourceLine` {string} The source line containing the top-level await.
27742792

27752793
<a id="ERR_REQUIRE_CYCLE_MODULE"></a>
27762794

‎doc/api/modules.md‎

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

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

325324
If support for loading ES modules using `require()` results in unexpected
326325
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
@@ -1699,20 +1699,36 @@ E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parent, locations) {
16991699
if (!getOptionValue('--experimental-print-required-tla')) {
17001700
message += ' To see where the top-level await comes from, use --experimental-print-required-tla.';
17011701
}
1702+
if (filename) {
1703+
message += `\nRequired module: ${filename}`;
1704+
}
17021705
if (parent) {
17031706
const { getRequireStack } = require('internal/modules/helpers');
17041707
const requireStack = getRequireStack(parent);
17051708
if (requireStack.length > 0) {
17061709
message += '\nRequire stack:\n- ' +
17071710
ArrayPrototypeJoin(requireStack, '\n- ');
17081711
}
1709-
this.requireStack = requireStack;
1712+
ObjectDefineProperty(this, 'requireStack', {
1713+
__proto__: null,
1714+
enumerable: false,
1715+
configurable: true,
1716+
writable: true,
1717+
value: requireStack,
1718+
});
17101719
}
17111720
if (locations && locations.length > 0) {
17121721
const { urlToFilename } = require('internal/modules/helpers');
17131722
const frames = ArrayPrototypeMap(locations, ({ url, line, column, sourceLine }) =>
1714-
`${urlToFilename(url)}:${line}\n\n${sourceLine}\n${StringPrototypeRepeat(' ', column)}^\n`);
1723+
`${urlToFilename(url)}:${line}\n\n${sourceLine}\n${StringPrototypeRepeat(' ', column - 1)}^\n`);
17151724
setArrowMessage(this, ArrayPrototypeJoin(frames, '\n'));
1725+
ObjectDefineProperty(this, 'topLevelAwaitLocations', {
1726+
__proto__: null,
1727+
enumerable: false,
1728+
configurable: true,
1729+
writable: true,
1730+
value: locations,
1731+
});
17161732
}
17171733
return message;
17181734
}, Error);

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -288,8 +288,8 @@ class ModuleLoader {
288288
const status = job.module.getStatus();
289289
debug('Module status', job, status);
290290
// hasAsyncGraph is available after module been instantiated.
291-
if (status >= kInstantiated && job.module.hasAsyncGraph) {
292-
job.throwAsyncGraphError(parent);
291+
if (status >= kInstantiated) {
292+
job.throwIfAsyncGraph(parent);
293293
}
294294
if (status === kEvaluated) {
295295
return { wrap: job.module, namespace: job.module.getNamespace() };
@@ -317,9 +317,11 @@ class ModuleLoader {
317317
}
318318
if (status !== kEvaluating) {
319319
assert(status === kUninstantiated, `Unexpected module status ${status}`);
320-
// A previous require() of the same graph may have bailed out before
321-
// instantiation because it contains top-level await.
322-
job.throwIfAsyncGraph(parent);
320+
// If we get here, either there's a race where the job is still being instantiated
321+
// by an in-flight import(), or the cached module previously encountered an
322+
// instantiation error during a prior load (e.g. due to a mismatched import).
323+
// TODO(joyeecheung): the current check is too broad. We should attempt to
324+
// get the potential instantiation error and throw it.
323325
throw new ERR_REQUIRE_ESM_RACE_CONDITION(filename, parentFilename, false);
324326
}
325327
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,
@@ -36,8 +38,10 @@ const {
3638
kUninstantiated,
3739
} = internalBinding('module_wrap');
3840
const {
41+
getPromiseDetails,
3942
privateSymbols: {
4043
entry_point_module_private_symbol,
44+
module_source_private_symbol: kModuleSource,
4145
},
4246
} = internalBinding('util');
4347
/**
@@ -134,12 +138,12 @@ const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
134138
* @typedef {object} TopLevelAwaitLocation
135139
* @property {string} url URL of the module containing the top-level await.
136140
* @property {number} line 1-based line number of the top-level await.
137-
* @property {number} column 0-based column number of the top-level await.
141+
* @property {number} column 1-based column number of the top-level await.
138142
* @property {string} sourceLine The source line containing the top-level await.
139143
*/
140144

141145
/**
142-
* Locate the top-level awaits in the given module by parsing the source with acron.
146+
* Locate the top-level awaits in the given module by parsing the source with acorn.
143147
* @param {string} source Module source code.
144148
* @returns {object[]} The acorn AST nodes of the top-level awaits, in source order.
145149
*/
@@ -173,27 +177,62 @@ function findTopLevelAwait(source) {
173177
return found;
174178
}
175179

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

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

320316
/**
@@ -529,19 +525,15 @@ class ModuleJob extends ModuleJobBase {
529525
status = this.module.getStatus();
530526
}
531527
if (status === kInstantiated || status === kErrored) {
532-
if (this.module.hasAsyncGraph) {
533-
this.throwAsyncGraphError(parent);
534-
}
528+
this.throwIfAsyncGraph(parent);
535529
if (status === kInstantiated) {
536530
setHasStartedUserESMExecution();
537531
const namespace = this.module.evaluateSync();
538532
return { __proto__: null, module: this.module, namespace };
539533
}
540534
throw this.module.getError();
541535
} else if (status === kEvaluating || status === kEvaluated) {
542-
if (this.module.hasAsyncGraph) {
543-
this.throwAsyncGraphError(parent);
544-
}
536+
this.throwIfAsyncGraph(parent);
545537
// kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles.
546538
// Allow it for now, since we only need to ban ESM <-> CJS cycles which would be
547539
// detected earlier during the linking phase, though the CJS handling in the ESM
@@ -637,12 +629,11 @@ class ModuleJobSync extends ModuleJobBase {
637629
}
638630
return { __proto__: null, module: this.module };
639631
} else if (status === kInstantiated || status === kUninstantiated) {
640-
// The require() of this (synchronously linked) module bailed out: either
641-
// it was rejected for containing top-level await after instantiation
642-
// (kInstantiated), or its instantiation failed and left it uninstantiated
643-
// (kUninstantiated, e.g. a missing named export). When it's reached via async
644-
// run() from import, finish the instantiation and evaluate it asynchronously,
645-
// re-throwing any instantiation error.
632+
// If we get here, the module was initially required and is now being imported.
633+
// The require() module failed either because the graph has TLA (kInstantiated),
634+
// or instantiation failed (kUninstantiated, e.g. missing named export).
635+
// Try finishing the instantiation - if it succeeds, proceed to evaluation,
636+
// otherwise the branch below re-throw any instantiation error.
646637
if (status === kUninstantiated) {
647638
this.module.instantiate();
648639
}
@@ -668,9 +659,7 @@ class ModuleJobSync extends ModuleJobBase {
668659
// On the deprecated async loader hook worker thread, dependencies linked by an
669660
// earlier import may not be walkable synchronously, so double-check with
670661
// V8 now that the graph is instantiated.
671-
if (this.module.hasAsyncGraph) {
672-
this.throwAsyncGraphError(parent);
673-
}
662+
this.throwIfAsyncGraph(parent);
674663
setHasStartedUserESMExecution();
675664
try {
676665
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 {
@@ -332,7 +333,7 @@ function compileSourceTextModule(url, source, cascadedLoader, context = kEmptyOb
332333
// only serves as a shortcut.
333334
if (wrap.hasTopLevelAwait &&
334335
getOptionValue('--experimental-print-required-tla')) {
335-
wrap.source = source;
336+
wrap[kModuleSource] = source;
336337
}
337338

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

0 commit comments

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