Skip to content

Commit 35cbfae

Browse files
lukastaegertlukastaegert
authored
Track unfinished hook actions as regular errors (#4434)
1 parent 511e9ae commit 35cbfae

15 files changed

Lines changed: 173 additions & 83 deletions

File tree

‎browser/hookActions.ts‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,8 @@
1-
export function addUnresolvedAction(_actionTuple: [string, string, Parameters<any>]): void {}
1+
import { PluginDriver } from '../src/utils/PluginDriver';
22

3-
export function resolveAction(_actionTuple: [string, string, Parameters<any>]): void {}
3+
export function catchUnfinishedHookActions<T>(
4+
_pluginDriver: PluginDriver,
5+
callback: () => Promise<T>
6+
): Promise<T> {
7+
return callback();
8+
}

‎src/rollup/rollup.ts‎

Lines changed: 32 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import type { PluginDriver } from '../utils/PluginDriver';
55
import { ensureArray } from '../utils/ensureArray';
66
import { errAlreadyClosed, errCannotEmitFromOptionsHook, error } from '../utils/error';
77
import { promises as fs } from '../utils/fs';
8+
import { catchUnfinishedHookActions } from '../utils/hookActions';
89
import { normalizeInputOptions } from '../utils/options/normalizeInputOptions';
910
import { normalizeOutputOptions } from '../utils/options/normalizeOutputOptions';
1011
import type { GenericConfigObject } from '../utils/options/options';
@@ -48,20 +49,21 @@ export async function rollupInternal(
4849

4950
timeStart('BUILD', 1);
5051

51-
try {
52-
await graph.pluginDriver.hookParallel('buildStart', [inputOptions]);
53-
await graph.build();
54-
} catch (err: any) {
55-
const watchFiles = Object.keys(graph.watchFiles);
56-
if (watchFiles.length > 0) {
57-
err.watchFiles = watchFiles;
52+
await catchUnfinishedHookActions(graph.pluginDriver, async () => {
53+
try {
54+
await graph.pluginDriver.hookParallel('buildStart', [inputOptions]);
55+
await graph.build();
56+
} catch (err: any) {
57+
const watchFiles = Object.keys(graph.watchFiles);
58+
if (watchFiles.length > 0) {
59+
err.watchFiles = watchFiles;
60+
}
61+
await graph.pluginDriver.hookParallel('buildEnd', [err]);
62+
await graph.pluginDriver.hookParallel('closeBundle', []);
63+
throw err;
5864
}
59-
await graph.pluginDriver.hookParallel('buildEnd', [err]);
60-
await graph.pluginDriver.hookParallel('closeBundle', []);
61-
throw err;
62-
}
63-
64-
await graph.pluginDriver.hookParallel('buildEnd', []);
65+
await graph.pluginDriver.hookParallel('buildEnd', []);
66+
});
6567

6668
timeEnd('BUILD', 1);
6769

@@ -144,7 +146,7 @@ function normalizePlugins(plugins: readonly Plugin[], anonymousPrefix: string):
144146
});
145147
}
146148

147-
async function handleGenerateWrite(
149+
function handleGenerateWrite(
148150
isWrite: boolean,
149151
inputOptions: NormalizedInputOptions,
150152
unsetInputOptions: ReadonlySet<string>,
@@ -161,19 +163,23 @@ async function handleGenerateWrite(
161163
inputOptions,
162164
unsetInputOptions
163165
);
164-
const bundle = new Bundle(outputOptions, unsetOptions, inputOptions, outputPluginDriver, graph);
165-
const generated = await bundle.generate(isWrite);
166-
if (isWrite) {
167-
if (!outputOptions.dir && !outputOptions.file) {
168-
return error({
169-
code: 'MISSING_OPTION',
170-
message: 'You must specify "output.file" or "output.dir" for the build.'
171-
});
166+
return catchUnfinishedHookActions(outputPluginDriver, async () => {
167+
const bundle = new Bundle(outputOptions, unsetOptions, inputOptions, outputPluginDriver, graph);
168+
const generated = await bundle.generate(isWrite);
169+
if (isWrite) {
170+
if (!outputOptions.dir && !outputOptions.file) {
171+
return error({
172+
code: 'MISSING_OPTION',
173+
message: 'You must specify "output.file" or "output.dir" for the build.'
174+
});
175+
}
176+
await Promise.all(
177+
Object.values(generated).map(chunk => writeOutputFile(chunk, outputOptions))
178+
);
179+
await outputPluginDriver.hookParallel('writeBundle', [outputOptions, generated]);
172180
}
173-
await Promise.all(Object.values(generated).map(chunk => writeOutputFile(chunk, outputOptions)));
174-
await outputPluginDriver.hookParallel('writeBundle', [outputOptions, generated]);
175-
}
176-
return createOutput(generated);
181+
return createOutput(generated);
182+
});
177183
}
178184

179185
function getOutputOptionsAndPluginDriver(

‎src/utils/PluginDriver.ts‎

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import type {
2222
import { FileEmitter } from './FileEmitter';
2323
import { getPluginContext } from './PluginContext';
2424
import { errInputHookInOutputPlugin, error } from './error';
25-
import { addUnresolvedAction, resolveAction } from './hookActions';
2625
import { throwPluginError, warnDeprecatedHooks } from './pluginUtils';
2726

2827
/**
@@ -70,6 +69,8 @@ function throwInvalidHookError(hookName: string, pluginName: string): never {
7069
});
7170
}
7271

72+
export type HookAction = [plugin: string, hook: string, args: unknown[]];
73+
7374
export class PluginDriver {
7475
public readonly emitFile: EmitFile;
7576
public finaliseAssets: () => void;
@@ -84,6 +85,7 @@ export class PluginDriver {
8485
private readonly pluginCache: Record<string, SerializablePluginCache> | undefined;
8586
private readonly pluginContexts: ReadonlyMap<Plugin, PluginContext>;
8687
private readonly plugins: readonly Plugin[];
88+
private readonly unfulfilledActions = new Set<HookAction>();
8789

8890
constructor(
8991
private readonly graph: Graph,
@@ -128,6 +130,10 @@ export class PluginDriver {
128130
return new PluginDriver(this.graph, this.options, plugins, this.pluginCache, this);
129131
}
130132

133+
getUnfulfilledHookActions(): Set<HookAction> {
134+
return this.unfulfilledActions;
135+
}
136+
131137
// chains, first non-null result stops and returns
132138
hookFirst<H extends AsyncPluginHooks & FirstPluginHooks>(
133139
hookName: H,
@@ -328,23 +334,22 @@ export class PluginDriver {
328334
// exit with a successful 0 return code but without producing any
329335
// output, errors or warnings.
330336
action = [plugin.name, hookName, args];
331-
addUnresolvedAction(action);
337+
this.unfulfilledActions.add(action);
332338

333339
// Although it would be more elegant to just return hookResult here
334340
// and put the .then() handler just above the .catch() handler below,
335341
// doing so would subtly change the defacto async event dispatch order
336342
// which at least one test and some plugins in the wild may depend on.
337-
const promise = Promise.resolve(hookResult);
338-
return promise.then(() => {
343+
return Promise.resolve(hookResult).then(result => {
339344
// action was fulfilled
340-
resolveAction(action as [string, string, Parameters<any>]);
341-
return promise;
345+
this.unfulfilledActions.delete(action!);
346+
return result;
342347
});
343348
})
344349
.catch(err => {
345350
if (action !== null) {
346351
// action considered to be fulfilled since error being handled
347-
resolveAction(action);
352+
this.unfulfilledActions.delete(action);
348353
}
349354
return throwPluginError(err, plugin.name, { hook: hookName });
350355
});

‎src/utils/hookActions.ts‎

Lines changed: 24 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,7 @@
11
import process from 'process';
2+
import { HookAction, PluginDriver } from './PluginDriver';
23

3-
const unfulfilledActions = new Set<[string, string, Parameters<any>]>();
4-
5-
export function addUnresolvedAction(actionTuple: [string, string, Parameters<any>]): void {
6-
unfulfilledActions.add(actionTuple);
7-
}
8-
9-
export function resolveAction(actionTuple: [string, string, Parameters<any>]): void {
10-
unfulfilledActions.delete(actionTuple);
11-
}
12-
13-
function formatAction([pluginName, hookName, args]: [string, string, Parameters<any>]): string {
4+
function formatAction([pluginName, hookName, args]: HookAction): string {
145
const action = `(${pluginName}) ${hookName}`;
156
const s = JSON.stringify;
167
switch (hookName) {
@@ -28,13 +19,25 @@ function formatAction([pluginName, hookName, args]: [string, string, Parameters<
2819
return action;
2920
}
3021

31-
process.on('exit', () => {
32-
if (unfulfilledActions.size) {
33-
let err = '[!] Error: unfinished hook action(s) on exit:\n';
34-
for (const action of unfulfilledActions) {
35-
err += formatAction(action) + '\n';
36-
}
37-
console.error('%s', err);
38-
process.exitCode = 1;
39-
}
40-
});
22+
export async function catchUnfinishedHookActions<T>(
23+
pluginDriver: PluginDriver,
24+
callback: () => Promise<T>
25+
): Promise<T> {
26+
let handleEmptyEventLoop: () => void;
27+
const emptyEventLoopPromise = new Promise<T>((_, reject) => {
28+
handleEmptyEventLoop = () => {
29+
const unfulfilledActions = pluginDriver.getUnfulfilledHookActions();
30+
reject(
31+
new Error(
32+
`Unexpected early exit. This happens when Promises returned by plugins cannot resolve. Unfinished hook action(s) on exit:\n` +
33+
[...unfulfilledActions].map(formatAction).join('\n')
34+
)
35+
);
36+
};
37+
process.once('beforeExit', handleEmptyEventLoop);
38+
});
39+
40+
const result = await Promise.race([callback(), emptyEventLoopPromise]);
41+
process.off('beforeExit', handleEmptyEventLoop!);
42+
return result;
43+
}

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

Lines changed: 4 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,16 @@
11
const assert = require('assert');
2-
const { assertIncludes } = require('../../../utils.js');
2+
const { assertIncludes, assertDoesNotInclude } = require('../../../utils.js');
33

44
module.exports = {
5-
description: 'does not swallow errors on unfulfilled hooks actions',
5+
description: 'does not show unfulfilled hook actions if there are errors',
66
minNodeVersion: 13,
77
command: 'node build.mjs',
88
after(err) {
99
// exit code check has to be here as error(err) is only called upon failure
10-
assert.strictEqual(err && err.code, 1);
11-
},
12-
error() {
13-
// do not abort test upon error
14-
return true;
10+
assert.strictEqual(err, null);
1511
},
1612
stderr(stderr) {
17-
assertIncludes(stderr, '[!] Error: unfinished hook action(s) on exit');
18-
assertIncludes(stderr, '(test) transform');
1913
assertIncludes(stderr, 'Error: Error must be displayed.');
14+
assertDoesNotInclude(stderr, 'Unfinished');
2015
}
2116
};

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

Lines changed: 20 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -3,21 +3,25 @@ import { rollup } from 'rollup';
33
let resolveA;
44
const waitForA = new Promise(resolve => (resolveA = resolve));
55

6-
// The error must not be swallowed when using top-level-await
7-
await rollup({
8-
input: './index.js',
9-
plugins: [
10-
{
11-
name: 'test',
12-
transform(code, id) {
13-
if (id.endsWith('a.js')) {
14-
resolveA();
15-
return new Promise(() => {});
16-
}
17-
if (id.endsWith('b.js')) {
18-
return waitForA.then(() => Promise.reject(new Error('Error must be displayed.')))
6+
try {
7+
// The error must not be swallowed when using top-level-await
8+
await rollup({
9+
input: './index.js',
10+
plugins: [
11+
{
12+
name: 'test',
13+
transform(code, id) {
14+
if (id.endsWith('a.js')) {
15+
resolveA();
16+
return new Promise(() => {});
17+
}
18+
if (id.endsWith('b.js')) {
19+
return waitForA.then(() => Promise.reject(new Error('Error must be displayed.')));
20+
}
1921
}
2022
}
21-
}
22-
]
23-
});
23+
]
24+
});
25+
} catch (err) {
26+
console.error(err);
27+
}

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
const assert = require('assert');
2+
const { assertDoesNotInclude } = require('../../../utils');
3+
const { assertIncludes } = require('../../../utils.js');
4+
5+
module.exports = {
6+
description:
7+
'does not show unfulfilled hook actions when exiting manually with a non-zero exit code',
8+
command: 'rollup -c --silent',
9+
after(err) {
10+
assert.strictEqual(err && err.code, 1);
11+
},
12+
error() {
13+
// do not abort test upon error
14+
return true;
15+
},
16+
stderr(stderr) {
17+
assertDoesNotInclude(stderr, 'Unfinished');
18+
assertIncludes(stderr, 'Manual exit');
19+
}
20+
};

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

Whitespace-only changes.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
console.log('a');

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
console.log('b');

0 commit comments

Comments
 (0)