module: throw error when re-runing errored module jobs · nodejs/node@515b581 · GitHub

nodejs / node Public

• Notifications You must be signed in to change notification settings
• Fork 36.6k
• Star 120k

• Code
• Issues 634
• Pull requests 749
• Actions
• Projects
• Security and quality
• Insights

Additional navigation options

Commit 515b581

Browse filesBrowse the repository at this point in the historyBrowse files

joyeecheung

authored and

targos

committed

module: throw error when re-runing errored module jobs

Re-evaluating an errored ESM should lead to rejecting the rejection again - this is also the case when importing it twice. In the case of retrying with require after import, just throw the cached error. Drive-by: add some debug logs. PR-URL: #58957 Fixes: #58945 Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>

1 parent c9ded6b commit 515b581
Copy full SHA for 515b581

4 files changed

+45-2Lines changed: 45 additions & 2 deletions

File tree

Expand file treeCollapse file tree

Open diff view settings

Filter options

• lib/internal/modules/esm
  • loader.js
  • module_job.js
• test/es-module
  • test-import-module-retry-require-errored.js
  • test-require-module-retry-import-errored-2.js

Expand file treeCollapse file tree

Open diff view settings

Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/loader.js

+4Lines changed: 4 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -43,6 +43,7 @@ const {
43	43	kEvaluating,
44	44	kEvaluationPhase,
45	45	kInstantiated,
	46	+ kErrored,
46	47	kSourcePhase,
47	48	throwIfPromiseRejected,
48	49	} = internalBinding('module_wrap');
@@ -402,6 +403,9 @@ class ModuleLoader {
402	403	mod[kRequiredModuleSymbol] = job.module;
403	404	const { namespace } = job.runSync(parent);
404	405	return { wrap: job.module, namespace: namespace || job.module.getNamespace() };
	406	+ } else if (status === kErrored) {
	407	+ // If the module was previously imported and errored, throw the error.
	408	+ throw job.module.getError();
405	409	}
406	410	// When the cached async job have already encountered a linking
407	411	// error that gets wrapped into a rejection, but is still later

Collapse file

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

Copy file name to clipboardExpand all lines: lib/internal/modules/esm/module_job.js

+8-2Lines changed: 8 additions & 2 deletions

Original file line number	Diff line number	Diff line change
@@ -323,7 +323,7 @@ class ModuleJob extends ModuleJobBase {
323	323	assert(this.module instanceof ModuleWrap);
324	324	let status = this.module.getStatus();
325	325
326		- debug('ModuleJob.runSync', this.module);
	326	+ debug('ModuleJob.runSync()', status, this.module);
327	327	// FIXME(joyeecheung): this cannot fully handle < kInstantiated. Make the linking
328	328	// fully synchronous instead.
329	329	if (status === kUninstantiated) {
@@ -358,6 +358,7 @@ class ModuleJob extends ModuleJobBase {
358	358	}
359	359
360	360	async run(isEntryPoint = false) {
	361	+ debug('ModuleJob.run()', this.module);
361	362	assert(this.phase === kEvaluationPhase);
362	363	await this.#instantiate();
363	364	if (isEntryPoint) {
@@ -461,7 +462,11 @@ class ModuleJobSync extends ModuleJobBase {
461	462	assert(this.phase === kEvaluationPhase);
462	463	// This path is hit by a require'd module that is imported again.
463	464	const status = this.module.getStatus();
464		- if (status > kInstantiated) {
	465	+ debug('ModuleJobSync.run()', status, this.module);
	466	+ // If the module was previously required and errored, reject from import() again.
	467	+ if (status === kErrored) {
	468	+ throw this.module.getError();
	469	+ } else if (status > kInstantiated) {
465	470	if (this.evaluationPromise) {
466	471	await this.evaluationPromise;
467	472	}
@@ -482,6 +487,7 @@ class ModuleJobSync extends ModuleJobBase {
482	487	}
483	488
484	489	runSync(parent) {
	490	+ debug('ModuleJobSync.runSync()', this.module);
485	491	assert(this.phase === kEvaluationPhase);
486	492	// TODO(joyeecheung): add the error decoration logic from the async instantiate.
487	493	this.module.async = this.module.instantiateSync();

Collapse file

‎test/es-module/test-import-module-retry-require-errored.js‎

Copy file name to clipboard

+17Lines changed: 17 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -0,0 +1,17 @@
	1	+ // This tests that after failing to import an ESM that rejects,
	2	+ // retrying with require() still throws.
	3	+
	4	+ 'use strict';
	5	+ const common = require('../common');
	6	+ const assert = require('assert');
	7	+
	8	+ (async () => {
	9	+ await assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), {
	10	+ message: 'test',
	11	+ });
	12	+ assert.throws(() => {
	13	+ require('../fixtures/es-modules/throw-error.mjs');
	14	+ }, {
	15	+ message: 'test',
	16	+ });
	17	+ })().catch(common.mustNotCall());

Collapse file

‎test/es-module/test-require-module-retry-import-errored-2.js‎

Copy file name to clipboard

+16Lines changed: 16 additions & 0 deletions

Original file line number	Diff line number	Diff line change
@@ -0,0 +1,16 @@
	1	+ // This tests that after failing to require an ESM that throws,
	2	+ // retrying with import() still rejects.
	3	+
	4	+ 'use strict';
	5	+ const common = require('../common');
	6	+ const assert = require('assert');
	7	+
	8	+ assert.throws(() => {
	9	+ require('../fixtures/es-modules/throw-error.mjs');
	10	+ }, {
	11	+ message: 'test',
	12	+ });
	13	+
	14	+ assert.rejects(import('../fixtures/es-modules/throw-error.mjs'), {
	15	+ message: 'test',
	16	+ }).catch(common.mustNotCall());

0 commit comments

Comments

0 (0)