From bb96097fc6768c83f58d1e4876493c666e153757 Mon Sep 17 00:00:00 2001 From: kzc Date: Fri, 17 Dec 2021 18:32:49 -0500 Subject: [PATCH] Detect unfulfilled async hook actions and report error on exit * 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. --- browser/hookActions.ts | 3 ++ build-plugins/replace-browser-modules.ts | 3 ++ src/utils/PluginDriver.ts | 35 +++++++++++++++++- src/utils/hookActions.ts | 37 +++++++++++++++++++ test/cli/index.js | 2 +- .../unfulfilled-hook-actions/_config.js | 24 ++++++++++++ .../unfulfilled-hook-actions/_expected.js | 0 .../cli/samples/unfulfilled-hook-actions/a.js | 1 + .../cli/samples/unfulfilled-hook-actions/b.js | 1 + .../unfulfilled-hook-actions/buggy-plugin.js | 33 +++++++++++++++++ .../cli/samples/unfulfilled-hook-actions/c.js | 1 + .../cli/samples/unfulfilled-hook-actions/d.js | 1 + .../samples/unfulfilled-hook-actions/main.js | 5 +++ 13 files changed, 143 insertions(+), 3 deletions(-) create mode 100644 browser/hookActions.ts create mode 100644 src/utils/hookActions.ts create mode 100644 test/cli/samples/unfulfilled-hook-actions/_config.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/_expected.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/a.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/b.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/buggy-plugin.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/c.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/d.js create mode 100644 test/cli/samples/unfulfilled-hook-actions/main.js diff --git a/browser/hookActions.ts b/browser/hookActions.ts new file mode 100644 index 00000000000..015f6ccb405 --- /dev/null +++ b/browser/hookActions.ts @@ -0,0 +1,3 @@ +export function addUnresolvedAction(_actionTuple: [string, string, Parameters]): void {} + +export function resolveAction(_actionTuple: [string, string, Parameters]): void {} diff --git a/build-plugins/replace-browser-modules.ts b/build-plugins/replace-browser-modules.ts index 9ffb7a8cc07..39c9acafe2a 100644 --- a/build-plugins/replace-browser-modules.ts +++ b/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: diff --git a/src/utils/PluginDriver.ts b/src/utils/PluginDriver.ts index 0035a40949a..5913fbeed04 100644 --- a/src/utils/PluginDriver.ts +++ b/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] | 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]); + 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 }); + }); } /** diff --git a/src/utils/hookActions.ts b/src/utils/hookActions.ts new file mode 100644 index 00000000000..eca7b630545 --- /dev/null +++ b/src/utils/hookActions.ts @@ -0,0 +1,37 @@ +const unfulfilledActions: Set<[string, string, Parameters]> = new Set(); + +export function addUnresolvedAction(actionTuple: [string, string, Parameters]): void { + unfulfilledActions.add(actionTuple); +} + +export function resolveAction(actionTuple: [string, string, Parameters]): void { + unfulfilledActions.delete(actionTuple); +} + +function formatAction([pluginName, hookName, args]: [string, string, Parameters]): 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); + } +}); diff --git a/test/cli/index.js b/test/cli/index.js index 5401bf699a0..4bbd525a431 100644 --- a/test/cli/index.js +++ b/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); diff --git a/test/cli/samples/unfulfilled-hook-actions/_config.js b/test/cli/samples/unfulfilled-hook-actions/_config.js new file mode 100644 index 00000000000..7336c290a29 --- /dev/null +++ b/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'); + } +}; diff --git a/test/cli/samples/unfulfilled-hook-actions/_expected.js b/test/cli/samples/unfulfilled-hook-actions/_expected.js new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/cli/samples/unfulfilled-hook-actions/a.js b/test/cli/samples/unfulfilled-hook-actions/a.js new file mode 100644 index 00000000000..8609d075540 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions/a.js @@ -0,0 +1 @@ +console.log('a'); diff --git a/test/cli/samples/unfulfilled-hook-actions/b.js b/test/cli/samples/unfulfilled-hook-actions/b.js new file mode 100644 index 00000000000..eeb313a0347 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions/b.js @@ -0,0 +1 @@ +console.log('b'); diff --git a/test/cli/samples/unfulfilled-hook-actions/buggy-plugin.js b/test/cli/samples/unfulfilled-hook-actions/buggy-plugin.js new file mode 100644 index 00000000000..4666e3d1e6e --- /dev/null +++ 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(() => {}); + } + }; +} diff --git a/test/cli/samples/unfulfilled-hook-actions/c.js b/test/cli/samples/unfulfilled-hook-actions/c.js new file mode 100644 index 00000000000..c7857d5f484 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions/c.js @@ -0,0 +1 @@ +console.log('c'); diff --git a/test/cli/samples/unfulfilled-hook-actions/d.js b/test/cli/samples/unfulfilled-hook-actions/d.js new file mode 100644 index 00000000000..3c130f85474 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions/d.js @@ -0,0 +1 @@ +console.log('d'); diff --git a/test/cli/samples/unfulfilled-hook-actions/main.js b/test/cli/samples/unfulfilled-hook-actions/main.js new file mode 100644 index 00000000000..f0d40cba75c --- /dev/null +++ b/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');