From 35cbfaeebb85cfb855494a896f71d282cf1184a1 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Mon, 14 Mar 2022 06:19:48 +0100 Subject: [PATCH] Track unfinished hook actions as regular errors (#4434) --- browser/hookActions.ts | 9 ++- src/rollup/rollup.ts | 58 ++++++++++--------- src/utils/PluginDriver.ts | 19 +++--- src/utils/hookActions.ts | 45 +++++++------- .../unfulfilled-hook-actions-error/_config.js | 13 ++--- .../unfulfilled-hook-actions-error/build.mjs | 36 +++++++----- .../_config.js | 20 +++++++ .../_expected.js | 0 .../unfulfilled-hook-actions-manual-exit/a.js | 1 + .../unfulfilled-hook-actions-manual-exit/b.js | 1 + .../unfulfilled-hook-actions-manual-exit/c.js | 1 + .../index.js | 3 + .../rollup.config.js | 29 ++++++++++ .../unfulfilled-hook-actions/_config.js | 6 +- test/utils.js | 15 ++++- 15 files changed, 173 insertions(+), 83 deletions(-) create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/_config.js create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/_expected.js create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/a.js create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/b.js create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/c.js create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/index.js create mode 100644 test/cli/samples/unfulfilled-hook-actions-manual-exit/rollup.config.js diff --git a/browser/hookActions.ts b/browser/hookActions.ts index 015f6ccb405..1f43a0fefb1 100644 --- a/browser/hookActions.ts +++ b/browser/hookActions.ts @@ -1,3 +1,8 @@ -export function addUnresolvedAction(_actionTuple: [string, string, Parameters]): void {} +import { PluginDriver } from '../src/utils/PluginDriver'; -export function resolveAction(_actionTuple: [string, string, Parameters]): void {} +export function catchUnfinishedHookActions( + _pluginDriver: PluginDriver, + callback: () => Promise +): Promise { + return callback(); +} diff --git a/src/rollup/rollup.ts b/src/rollup/rollup.ts index 68cd3dd04d6..f1981f7f1d5 100644 --- a/src/rollup/rollup.ts +++ b/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, @@ -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( diff --git a/src/utils/PluginDriver.ts b/src/utils/PluginDriver.ts index d4e8de71dfa..a436bc35586 100644 --- a/src/utils/PluginDriver.ts +++ b/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 | undefined; private readonly pluginContexts: ReadonlyMap; private readonly plugins: readonly Plugin[]; + private readonly unfulfilledActions = new Set(); 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 { + return this.unfulfilledActions; + } + // chains, first non-null result stops and returns hookFirst( 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]); - 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 }); }); diff --git a/src/utils/hookActions.ts b/src/utils/hookActions.ts index 3e669c2a134..69269bbb27f 100644 --- a/src/utils/hookActions.ts +++ b/src/utils/hookActions.ts @@ -1,16 +1,7 @@ import process from 'process'; +import { HookAction, PluginDriver } from './PluginDriver'; -const unfulfilledActions = new Set<[string, string, Parameters]>(); - -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 { +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( + pluginDriver: PluginDriver, + callback: () => Promise +): Promise { + let handleEmptyEventLoop: () => void; + const emptyEventLoopPromise = new Promise((_, 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; +} diff --git a/test/cli/samples/unfulfilled-hook-actions-error/_config.js b/test/cli/samples/unfulfilled-hook-actions-error/_config.js index 93b50f6fb1b..39bea541f9f 100644 --- a/test/cli/samples/unfulfilled-hook-actions-error/_config.js +++ b/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'); } }; diff --git a/test/cli/samples/unfulfilled-hook-actions-error/build.mjs b/test/cli/samples/unfulfilled-hook-actions-error/build.mjs index 4d7bdff4edb..9b2bf0f535e 100644 --- a/test/cli/samples/unfulfilled-hook-actions-error/build.mjs +++ b/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); +} diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/_config.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/_config.js new file mode 100644 index 00000000000..efdec0e2f2c --- /dev/null +++ b/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'); + } +}; diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/_expected.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/_expected.js new file mode 100644 index 00000000000..e69de29bb2d diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/a.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/a.js new file mode 100644 index 00000000000..8609d075540 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions-manual-exit/a.js @@ -0,0 +1 @@ +console.log('a'); diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/b.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/b.js new file mode 100644 index 00000000000..eeb313a0347 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions-manual-exit/b.js @@ -0,0 +1 @@ +console.log('b'); diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/c.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/c.js new file mode 100644 index 00000000000..c7857d5f484 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions-manual-exit/c.js @@ -0,0 +1 @@ +console.log('c'); diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/index.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/index.js new file mode 100644 index 00000000000..35468cc3f20 --- /dev/null +++ b/test/cli/samples/unfulfilled-hook-actions-manual-exit/index.js @@ -0,0 +1,3 @@ +import './a.js'; +import './b.js'; +import './c.js'; diff --git a/test/cli/samples/unfulfilled-hook-actions-manual-exit/rollup.config.js b/test/cli/samples/unfulfilled-hook-actions-manual-exit/rollup.config.js new file mode 100644 index 00000000000..71ef30e6734 --- /dev/null +++ b/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' + } +}; diff --git a/test/cli/samples/unfulfilled-hook-actions/_config.js b/test/cli/samples/unfulfilled-hook-actions/_config.js index 7aa6f14577b..6d7f7e4ba94 100644 --- a/test/cli/samples/unfulfilled-hook-actions/_config.js +++ b/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"'); diff --git a/test/utils.js b/test/utils.js index 220c1047bda..0ac77d546ed 100644 --- a/test/utils.js +++ b/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;