Track unfinished hook actions as regular errors

browser/hookActions.ts

@@ -1,3 +1,8 @@
-export function addUnresolvedAction(_actionTuple: [string, string, Parameters<any>]): void {}
+import { PluginDriver } from '../src/utils/PluginDriver';
 
-export function resolveAction(_actionTuple: [string, string, Parameters<any>]): void {}
+export function catchUnfinishedHookActions<T>(
+	_pluginDriver: PluginDriver,
+	callback: () => Promise<T>
+): Promise<T> {
+	return callback();
+}

src/rollup/rollup.ts

@@ -5,6 +5,7 @@ import type { PluginDriver } from '../utils/PluginDriver';
 import { ensureArray } from '../utils/ensureArray';
 import { errAlreadyClosed, errCannotEmitFromOptionsHook, error } from '../utils/error';
 import { promises as fs } from '../utils/fs';
+import { catchUnfinishedHookActions } from '../utils/hookActions';
 import { normalizeInputOptions } from '../utils/options/normalizeInputOptions';
 import { normalizeOutputOptions } from '../utils/options/normalizeOutputOptions';
 import type { GenericConfigObject } from '../utils/options/options';
@@ -48,20 +49,21 @@ export async function rollupInternal(
 
 	timeStart('BUILD', 1);
 
-	try {
-		await graph.pluginDriver.hookParallel('buildStart', [inputOptions]);
-		await graph.build();
-	} catch (err: any) {
-		const watchFiles = Object.keys(graph.watchFiles);
-		if (watchFiles.length > 0) {
-			err.watchFiles = watchFiles;
+	await catchUnfinishedHookActions(graph.pluginDriver, async () => {
+		try {
+			await graph.pluginDriver.hookParallel('buildStart', [inputOptions]);
+			await graph.build();
+		} catch (err: any) {
+			const watchFiles = Object.keys(graph.watchFiles);
+			if (watchFiles.length > 0) {
+				err.watchFiles = watchFiles;
+			}
+			await graph.pluginDriver.hookParallel('buildEnd', [err]);
+			await graph.pluginDriver.hookParallel('closeBundle', []);
+			throw err;
 		}
-		await graph.pluginDriver.hookParallel('buildEnd', [err]);
-		await graph.pluginDriver.hookParallel('closeBundle', []);
-		throw err;
-	}
-
-	await graph.pluginDriver.hookParallel('buildEnd', []);
+		await graph.pluginDriver.hookParallel('buildEnd', []);
+	});
 
 	timeEnd('BUILD', 1);
 
@@ -144,7 +146,7 @@ function normalizePlugins(plugins: readonly Plugin[], anonymousPrefix: string):
 	});
 }
 
-async function handleGenerateWrite(
+function handleGenerateWrite(
 	isWrite: boolean,
 	inputOptions: NormalizedInputOptions,
 	unsetInputOptions: ReadonlySet<string>,
@@ -161,19 +163,23 @@ async function handleGenerateWrite(
 		inputOptions,
 		unsetInputOptions
 	);
-	const bundle = new Bundle(outputOptions, unsetOptions, inputOptions, outputPluginDriver, graph);
-	const generated = await bundle.generate(isWrite);
-	if (isWrite) {
-		if (!outputOptions.dir && !outputOptions.file) {
-			return error({
-				code: 'MISSING_OPTION',
-				message: 'You must specify "output.file" or "output.dir" for the build.'
-			});
+	return catchUnfinishedHookActions(outputPluginDriver, async () => {
+		const bundle = new Bundle(outputOptions, unsetOptions, inputOptions, outputPluginDriver, graph);
+		const generated = await bundle.generate(isWrite);
+		if (isWrite) {
+			if (!outputOptions.dir && !outputOptions.file) {
+				return error({
+					code: 'MISSING_OPTION',
+					message: 'You must specify "output.file" or "output.dir" for the build.'
+				});
+			}
+			await Promise.all(
+				Object.values(generated).map(chunk => writeOutputFile(chunk, outputOptions))
+			);
+			await outputPluginDriver.hookParallel('writeBundle', [outputOptions, generated]);
 		}
-		await Promise.all(Object.values(generated).map(chunk => writeOutputFile(chunk, outputOptions)));
-		await outputPluginDriver.hookParallel('writeBundle', [outputOptions, generated]);
-	}
-	return createOutput(generated);
+		return createOutput(generated);
+	});
 }
 
 function getOutputOptionsAndPluginDriver(

src/utils/PluginDriver.ts

@@ -22,7 +22,6 @@ import type {
 import { FileEmitter } from './FileEmitter';
 import { getPluginContext } from './PluginContext';
 import { errInputHookInOutputPlugin, error } from './error';
-import { addUnresolvedAction, resolveAction } from './hookActions';
 import { throwPluginError, warnDeprecatedHooks } from './pluginUtils';
 
 /**
@@ -70,6 +69,8 @@ function throwInvalidHookError(hookName: string, pluginName: string): never {
 	});
 }
 
+export type HookAction = [plugin: string, hook: string, args: unknown[]];
+
 export class PluginDriver {
 	public readonly emitFile: EmitFile;
 	public finaliseAssets: () => void;
@@ -84,6 +85,7 @@ export class PluginDriver {
 	private readonly pluginCache: Record<string, SerializablePluginCache> | undefined;
 	private readonly pluginContexts: ReadonlyMap<Plugin, PluginContext>;
 	private readonly plugins: readonly Plugin[];
+	private readonly unfulfilledActions = new Set<HookAction>();
 
 	constructor(
 		private readonly graph: Graph,
@@ -128,6 +130,10 @@ export class PluginDriver {
 		return new PluginDriver(this.graph, this.options, plugins, this.pluginCache, this);
 	}
 
+	getUnfulfilledHookActions(): Set<HookAction> {
+		return this.unfulfilledActions;
+	}
+
 	// chains, first non-null result stops and returns
 	hookFirst<H extends AsyncPluginHooks & FirstPluginHooks>(
 		hookName: H,
@@ -328,23 +334,22 @@ export class PluginDriver {
 				// exit with a successful 0 return code but without producing any
 				// output, errors or warnings.
 				action = [plugin.name, hookName, args];
-				addUnresolvedAction(action);
+				this.unfulfilledActions.add(action);
 
 				// Although it would be more elegant to just return hookResult here
 				// and put the .then() handler just above the .catch() handler below,
 				// doing so would subtly change the defacto async event dispatch order
 				// which at least one test and some plugins in the wild may depend on.
-				const promise = Promise.resolve(hookResult);
-				return promise.then(() => {
+				return Promise.resolve(hookResult).then(result => {
 					// action was fulfilled
-					resolveAction(action as [string, string, Parameters<any>]);
-					return promise;
+					this.unfulfilledActions.delete(action!);
+					return result;
 				});
 			})
 			.catch(err => {
 				if (action !== null) {
 					// action considered to be fulfilled since error being handled
-					resolveAction(action);
+					this.unfulfilledActions.delete(action);
 				}
 				return throwPluginError(err, plugin.name, { hook: hookName });
 			});

src/utils/hookActions.ts

@@ -1,16 +1,7 @@
 import process from 'process';
+import { HookAction, PluginDriver } from './PluginDriver';
 
-const unfulfilledActions = new Set<[string, string, Parameters<any>]>();
-
-export function addUnresolvedAction(actionTuple: [string, string, Parameters<any>]): void {
-	unfulfilledActions.add(actionTuple);
-}
-
-export function resolveAction(actionTuple: [string, string, Parameters<any>]): void {
-	unfulfilledActions.delete(actionTuple);
-}
-
-function formatAction([pluginName, hookName, args]: [string, string, Parameters<any>]): string {
+function formatAction([pluginName, hookName, args]: HookAction): string {
 	const action = `(${pluginName}) ${hookName}`;
 	const s = JSON.stringify;
 	switch (hookName) {
@@ -28,13 +19,25 @@ function formatAction([pluginName, hookName, args]: [string, string, Parameters<
 	return action;
 }
 
-process.on('exit', () => {
-	if (unfulfilledActions.size) {
-		let err = '[!] Error: unfinished hook action(s) on exit:\n';
-		for (const action of unfulfilledActions) {
-			err += formatAction(action) + '\n';
-		}
-		console.error('%s', err);
-		process.exitCode = 1;
-	}
-});
+export async function catchUnfinishedHookActions<T>(
+	pluginDriver: PluginDriver,
+	callback: () => Promise<T>
+): Promise<T> {
+	let handleEmptyEventLoop: () => void;
+	const emptyEventLoopPromise = new Promise<T>((_, reject) => {
+		handleEmptyEventLoop = () => {
+			const unfulfilledActions = pluginDriver.getUnfulfilledHookActions();
+			reject(
+				new Error(
+					`Unexpected early exit. This happens when Promises returned by plugins cannot resolve. Unfinished hook action(s) on exit:\n` +
+						[...unfulfilledActions].map(formatAction).join('\n')
+				)
+			);
+		};
+		process.once('beforeExit', handleEmptyEventLoop);
+	});
+
+	const result = await Promise.race([callback(), emptyEventLoopPromise]);
+	process.off('beforeExit', handleEmptyEventLoop!);
+	return result;
+}

test/cli/samples/unfulfilled-hook-actions-error/_config.js

@@ -1,21 +1,16 @@
 const assert = require('assert');
-const { assertIncludes } = require('../../../utils.js');
+const { assertIncludes, assertDoesNotInclude } = require('../../../utils.js');
 
 module.exports = {
-	description: 'does not swallow errors on unfulfilled hooks actions',
+	description: 'does not show unfulfilled hook actions if there are errors',
 	minNodeVersion: 13,
 	command: 'node build.mjs',
 	after(err) {
 		// exit code check has to be here as error(err) is only called upon failure
-		assert.strictEqual(err && err.code, 1);
-	},
-	error() {
-		// do not abort test upon error
-		return true;
+		assert.strictEqual(err, null);
 	},
 	stderr(stderr) {
-		assertIncludes(stderr, '[!] Error: unfinished hook action(s) on exit');
-		assertIncludes(stderr, '(test) transform');
 		assertIncludes(stderr, 'Error: Error must be displayed.');
+		assertDoesNotInclude(stderr, 'Unfinished');
 	}
 };

test/cli/samples/unfulfilled-hook-actions-error/build.mjs

@@ -3,21 +3,25 @@ import { rollup } from 'rollup';
 let resolveA;
 const waitForA = new Promise(resolve => (resolveA = resolve));
 
-// The error must not be swallowed when using top-level-await
-await rollup({
-	input: './index.js',
-	plugins: [
-		{
-			name: 'test',
-			transform(code, id) {
-				if (id.endsWith('a.js')) {
-					resolveA();
-					return new Promise(() => {});
-				}
-				if (id.endsWith('b.js')) {
-					return waitForA.then(() => Promise.reject(new Error('Error must be displayed.')))
+try {
+	// The error must not be swallowed when using top-level-await
+	await rollup({
+		input: './index.js',
+		plugins: [
+			{
+				name: 'test',
+				transform(code, id) {
+					if (id.endsWith('a.js')) {
+						resolveA();
+						return new Promise(() => {});
+					}
+					if (id.endsWith('b.js')) {
+						return waitForA.then(() => Promise.reject(new Error('Error must be displayed.')));
+					}
 				}
 			}
-		}
-	]
-});
+		]
+	});
+} catch (err) {
+	console.error(err);
+}

test/cli/samples/unfulfilled-hook-actions-manual-exit/_config.js

@@ -0,0 +1,20 @@
+const assert = require('assert');
+const { assertDoesNotInclude } = require('../../../utils');
+const { assertIncludes } = require('../../../utils.js');
+
+module.exports = {
+	description:
+		'does not show unfulfilled hook actions when exiting manually with a non-zero exit code',
+	command: 'rollup -c --silent',
+	after(err) {
+		assert.strictEqual(err && err.code, 1);
+	},
+	error() {
+		// do not abort test upon error
+		return true;
+	},
+	stderr(stderr) {
+		assertDoesNotInclude(stderr, 'Unfinished');
+		assertIncludes(stderr, 'Manual exit');
+	}
+};

test/cli/samples/unfulfilled-hook-actions-manual-exit/a.js

@@ -0,0 +1 @@
+console.log('a');

test/cli/samples/unfulfilled-hook-actions-manual-exit/b.js

@@ -0,0 +1 @@
+console.log('b');

test/cli/samples/unfulfilled-hook-actions-manual-exit/c.js

@@ -0,0 +1 @@
+console.log('c');

test/cli/samples/unfulfilled-hook-actions-manual-exit/index.js

@@ -0,0 +1,3 @@
+import './a.js';
+import './b.js';
+import './c.js';

test/cli/samples/unfulfilled-hook-actions-manual-exit/rollup.config.js

@@ -0,0 +1,29 @@
+const path = require('path');
+
+let resolveA;
+const waitForA = new Promise(resolve => (resolveA = resolve));
+
+module.exports = {
+	input: './index.js',
+	plugins: [
+		{
+			name: 'test',
+			transform(code, id) {
+				if (id.endsWith('a.js')) {
+					resolveA();
+					return new Promise(() => {});
+				}
+				if (id.endsWith('b.js')) {
+					return waitForA.then(() => {
+						console.error('Manual exit');
+						process.exit(1);
+						return new Promise(() => {});
+					});
+				}
+			}
+		}
+	],
+	output: {
+		format: 'es'
+	}
+};

test/cli/samples/unfulfilled-hook-actions/_config.js

@@ -13,7 +13,11 @@ module.exports = {
 		return true;
 	},
 	stderr(stderr) {
-		assertIncludes(stderr, '[!] Error: unfinished hook action(s) on exit');
+		console.error(stderr);
+		assertIncludes(
+			stderr,
+			'Error: Unexpected early exit. This happens when Promises returned by plugins cannot resolve. Unfinished hook action(s) on exit:'
+		);
 
 		// these unfulfilled async hook actions may occur in random order
 		assertIncludes(stderr, '(buggy-plugin) resolveId "./c.js" "main.js"');

test/utils.js

@@ -228,7 +228,20 @@ exports.assertIncludes = function assertIncludes(actual, expected) {
 	try {
 		assert.ok(
 			actual.includes(expected),
-			`${JSON.stringify(actual)}\nincludes\n${JSON.stringify(expected)}`
+			`${JSON.stringify(actual)}\nshould include\n${JSON.stringify(expected)}`
+		);
+	} catch (err) {
+		err.actual = actual;
+		err.expected = expected;
+		throw err;
+	}
+};
+
+exports.assertDoesNotInclude = function assertDoesNotInclude(actual, expected) {
+	try {
+		assert.ok(
+			!actual.includes(expected),
+			`${JSON.stringify(actual)}\nshould not include\n${JSON.stringify(expected)}`
 		);
 	} catch (err) {
 		err.actual = actual;