Detect unfulfilled async hook actions and report error on exit (#4320)

* Better user experience with buggy plugins that previously caused
  rollup to exit abruptly without any output, error or warning,
  and a zero process exit code incorrectly indicating success.

Co-authored-by: Lukas Taegert-Atkinson <lukastaegert@users.noreply.github.com>

browser/hookActions.ts

@@ -0,0 +1,3 @@
+export function addUnresolvedAction(_actionTuple: [string, string, Parameters<any>]): void {}
+
+export function resolveAction(_actionTuple: [string, string, Parameters<any>]): void {}

build-plugins/replace-browser-modules.ts

@@ -3,6 +3,7 @@ import { Plugin } from 'rollup';
 
 const ID_CRYPTO = path.resolve('src/utils/crypto');
 const ID_FS = path.resolve('src/utils/fs');
+const ID_HOOKACTIONS = path.resolve('src/utils/hookActions');
 const ID_PATH = path.resolve('src/utils/path');
 const ID_RESOLVEID = path.resolve('src/utils/resolveId');
 
@@ -17,6 +18,8 @@ export default function replaceBrowserModules(): Plugin {
 						return path.resolve('browser/crypto.ts');
 					case ID_FS:
 						return path.resolve('browser/fs.ts');
+					case ID_HOOKACTIONS:
+						return path.resolve('browser/hookActions.ts');
 					case ID_PATH:
 						return path.resolve('browser/path.ts');
 					case ID_RESOLVEID:

src/utils/PluginDriver.ts

@@ -22,6 +22,7 @@ import {
 import { FileEmitter } from './FileEmitter';
 import { getPluginContext } from './PluginContext';
 import { errInputHookInOutputPlugin, error } from './error';
+import { addUnresolvedAction, resolveAction } from './hookActions';
 import { throwPluginError, warnDeprecatedHooks } from './pluginUtils';
 
 /**
@@ -314,6 +315,7 @@ export class PluginDriver {
 			context = hookContext(context, plugin);
 		}
 
+		let action: [string, string, Parameters<any>] | null = null;
 		return Promise.resolve()
 			.then(() => {
 				// permit values allows values to be returned instead of a functional hook
@@ -322,9 +324,38 @@ export class PluginDriver {
 					return throwInvalidHookError(hookName, plugin.name);
 				}
 				// eslint-disable-next-line @typescript-eslint/ban-types
-				return (hook as Function).apply(context, args);
+				const hookResult = (hook as Function).apply(context, args);
+
+				if (!hookResult || !hookResult.then) {
+					// short circuit for non-thenables and non-Promises
+					return hookResult;
+				}
+
+				// Track pending hook actions to properly error out when
+				// unfulfilled promises cause rollup to abruptly and confusingly
+				// exit with a successful 0 return code but without producing any
+				// output, errors or warnings.
+				action = [plugin.name, hookName, args];
+				addUnresolvedAction(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(() => {
+					// action was fulfilled
+					resolveAction(action as [string, string, Parameters<any>]);
+					return promise;
+				});
 			})
-			.catch(err => throwPluginError(err, plugin.name, { hook: hookName }));
+			.catch(err => {
+				if (action !== null) {
+					// action considered to be fulfilled since error being handled
+					resolveAction(action);
+				}
+				return throwPluginError(err, plugin.name, { hook: hookName });
+			});
 	}
 
 	/**

src/utils/hookActions.ts

@@ -0,0 +1,37 @@
+const unfulfilledActions: Set<[string, string, Parameters<any>]> = new Set();
+
+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 {
+	let action = `(${pluginName}) ${hookName}`;
+	const s = JSON.stringify;
+	switch (hookName) {
+		case 'resolveId':
+			action += ` ${s(args[0])} ${s(args[1])}`;
+			break;
+		case 'load':
+			action += ` ${s(args[0])}`;
+			break;
+		case 'transform':
+			action += ` ${s(args[1])}`;
+			break;
+	}
+	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.exit(1);
+	}
+});

test/cli/index.js

@@ -35,7 +35,7 @@ runTestSuiteWithSamples(
 						env: { ...process.env, FORCE_COLOR: '0', ...config.env }
 					},
 					(err, code, stderr) => {
-						if (config.after) config.after();
+						if (config.after) config.after(err, code, stderr);
 						if (err && !err.killed) {
 							if (config.error) {
 								const shouldContinue = config.error(err);

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

@@ -0,0 +1,24 @@
+const assert = require('assert');
+const { assertIncludes } = require('../../../utils.js');
+
+module.exports = {
+	description: 'show errors with non-zero exit code for unfulfilled async plugin actions on exit',
+	command: 'rollup main.js -p ./buggy-plugin.js --silent',
+	after(err) {
+		// exit code check has to be here as error(err) is only called upon failure
+		assert.strictEqual(err && err.code, 1);
+	},
+	error(err) {
+		// do not abort test upon error
+		return true;
+	},
+	stderr(stderr) {
+		assertIncludes(stderr, '[!] Error: 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"');
+		assertIncludes(stderr, '(buggy-plugin) load "./b.js"');
+		assertIncludes(stderr, '(buggy-plugin) transform "./a.js"');
+		assertIncludes(stderr, '(buggy-plugin) moduleParsed');
+	}
+};

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

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

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

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

test/cli/samples/unfulfilled-hook-actions/buggy-plugin.js

@@ -0,0 +1,33 @@
+var path = require('path');
+
+function relative(id) {
+	if (id[0] != '/') return id;
+	if (id[0] != '\\') return id;
+	return './' + path.relative(process.cwd(), id);
+}
+
+module.exports = function() {
+	return {
+		name: 'buggy-plugin',
+		resolveId(id) {
+			if (id.includes('\0')) return;
+
+			// this action will never resolve or reject
+			if (id.endsWith('c.js')) return new Promise(() => {});
+
+			return relative(id);
+		},
+		load(id) {
+			// this action will never resolve or reject
+			if (id.endsWith('b.js')) return new Promise(() => {});
+		},
+		transform(code, id) {
+			// this action will never resolve or reject
+			if (id.endsWith('a.js')) return new Promise(() => {});
+		},
+		moduleParsed(mod) {
+			// this action will never resolve or reject
+			if (mod.id.endsWith('d.js')) return new Promise(() => {});
+		}
+	};
+}

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

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

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

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

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

@@ -0,0 +1,5 @@
+import './a.js';
+import './b.js';
+import './c.js';
+import './d.js';
+console.log('main');