Skip to content

Commit 0ceee1d

Browse files
kzclukastaegert
kzc
and
lukastaegert
authored
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 <[email protected]>
1 parent 2fea0d7 commit 0ceee1d

13 files changed

Lines changed: 143 additions & 3 deletions

File tree

‎browser/hookActions.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
export function addUnresolvedAction(_actionTuple: [string, string, Parameters<any>]): void {}
2+
3+
export function resolveAction(_actionTuple: [string, string, Parameters<any>]): void {}

‎build-plugins/replace-browser-modules.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { Plugin } from 'rollup';
33

44
const ID_CRYPTO = path.resolve('src/utils/crypto');
55
const ID_FS = path.resolve('src/utils/fs');
6+
const ID_HOOKACTIONS = path.resolve('src/utils/hookActions');
67
const ID_PATH = path.resolve('src/utils/path');
78
const ID_RESOLVEID = path.resolve('src/utils/resolveId');
89

@@ -17,6 +18,8 @@ export default function replaceBrowserModules(): Plugin {
1718
return path.resolve('browser/crypto.ts');
1819
case ID_FS:
1920
return path.resolve('browser/fs.ts');
21+
case ID_HOOKACTIONS:
22+
return path.resolve('browser/hookActions.ts');
2023
case ID_PATH:
2124
return path.resolve('browser/path.ts');
2225
case ID_RESOLVEID:

‎src/utils/PluginDriver.ts‎

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

2728
/**
@@ -314,6 +315,7 @@ export class PluginDriver {
314315
context = hookContext(context, plugin);
315316
}
316317

318+
let action: [string, string, Parameters<any>] | null = null;
317319
return Promise.resolve()
318320
.then(() => {
319321
// permit values allows values to be returned instead of a functional hook
@@ -322,9 +324,38 @@ export class PluginDriver {
322324
return throwInvalidHookError(hookName, plugin.name);
323325
}
324326
// eslint-disable-next-line @typescript-eslint/ban-types
325-
return (hook as Function).apply(context, args);
327+
const hookResult = (hook as Function).apply(context, args);
328+
329+
if (!hookResult || !hookResult.then) {
330+
// short circuit for non-thenables and non-Promises
331+
return hookResult;
332+
}
333+
334+
// Track pending hook actions to properly error out when
335+
// unfulfilled promises cause rollup to abruptly and confusingly
336+
// exit with a successful 0 return code but without producing any
337+
// output, errors or warnings.
338+
action = [plugin.name, hookName, args];
339+
addUnresolvedAction(action);
340+
341+
// Although it would be more elegant to just return hookResult here
342+
// and put the .then() handler just above the .catch() handler below,
343+
// doing so would subtly change the defacto async event dispatch order
344+
// which at least one test and some plugins in the wild may depend on.
345+
const promise = Promise.resolve(hookResult);
346+
return promise.then(() => {
347+
// action was fulfilled
348+
resolveAction(action as [string, string, Parameters<any>]);
349+
return promise;
350+
});
326351
})
327-
.catch(err => throwPluginError(err, plugin.name, { hook: hookName }));
352+
.catch(err => {
353+
if (action !== null) {
354+
// action considered to be fulfilled since error being handled
355+
resolveAction(action);
356+
}
357+
return throwPluginError(err, plugin.name, { hook: hookName });
358+
});
328359
}
329360

330361
/**

‎src/utils/hookActions.ts‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
const unfulfilledActions: Set<[string, string, Parameters<any>]> = new Set();
2+
3+
export function addUnresolvedAction(actionTuple: [string, string, Parameters<any>]): void {
4+
unfulfilledActions.add(actionTuple);
5+
}
6+
7+
export function resolveAction(actionTuple: [string, string, Parameters<any>]): void {
8+
unfulfilledActions.delete(actionTuple);
9+
}
10+
11+
function formatAction([pluginName, hookName, args]: [string, string, Parameters<any>]): string {
12+
let action = `(${pluginName}) ${hookName}`;
13+
const s = JSON.stringify;
14+
switch (hookName) {
15+
case 'resolveId':
16+
action += ` ${s(args[0])} ${s(args[1])}`;
17+
break;
18+
case 'load':
19+
action += ` ${s(args[0])}`;
20+
break;
21+
case 'transform':
22+
action += ` ${s(args[1])}`;
23+
break;
24+
}
25+
return action;
26+
}
27+
28+
process.on('exit', () => {
29+
if (unfulfilledActions.size) {
30+
let err = '[!] Error: unfinished hook action(s) on exit:\n';
31+
for (const action of unfulfilledActions) {
32+
err += formatAction(action) + '\n';
33+
}
34+
console.error('%s', err);
35+
process.exit(1);
36+
}
37+
});

‎test/cli/index.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ runTestSuiteWithSamples(
3535
env: { ...process.env, FORCE_COLOR: '0', ...config.env }
3636
},
3737
(err, code, stderr) => {
38-
if (config.after) config.after();
38+
if (config.after) config.after(err, code, stderr);
3939
if (err && !err.killed) {
4040
if (config.error) {
4141
const shouldContinue = config.error(err);

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
const assert = require('assert');
2+
const { assertIncludes } = require('../../../utils.js');
3+
4+
module.exports = {
5+
description: 'show errors with non-zero exit code for unfulfilled async plugin actions on exit',
6+
command: 'rollup main.js -p ./buggy-plugin.js --silent',
7+
after(err) {
8+
// exit code check has to be here as error(err) is only called upon failure
9+
assert.strictEqual(err && err.code, 1);
10+
},
11+
error(err) {
12+
// do not abort test upon error
13+
return true;
14+
},
15+
stderr(stderr) {
16+
assertIncludes(stderr, '[!] Error: unfinished hook action(s) on exit');
17+
18+
// these unfulfilled async hook actions may occur in random order
19+
assertIncludes(stderr, '(buggy-plugin) resolveId "./c.js" "main.js"');
20+
assertIncludes(stderr, '(buggy-plugin) load "./b.js"');
21+
assertIncludes(stderr, '(buggy-plugin) transform "./a.js"');
22+
assertIncludes(stderr, '(buggy-plugin) moduleParsed');
23+
}
24+
};

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

Whitespace-only changes.

‎test/cli/samples/unfulfilled-hook-actions/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/b.js‎

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

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
var path = require('path');
2+
3+
function relative(id) {
4+
if (id[0] != '/') return id;
5+
if (id[0] != '\\') return id;
6+
return './' + path.relative(process.cwd(), id);
7+
}
8+
9+
module.exports = function() {
10+
return {
11+
name: 'buggy-plugin',
12+
resolveId(id) {
13+
if (id.includes('\0')) return;
14+
15+
// this action will never resolve or reject
16+
if (id.endsWith('c.js')) return new Promise(() => {});
17+
18+
return relative(id);
19+
},
20+
load(id) {
21+
// this action will never resolve or reject
22+
if (id.endsWith('b.js')) return new Promise(() => {});
23+
},
24+
transform(code, id) {
25+
// this action will never resolve or reject
26+
if (id.endsWith('a.js')) return new Promise(() => {});
27+
},
28+
moduleParsed(mod) {
29+
// this action will never resolve or reject
30+
if (mod.id.endsWith('d.js')) return new Promise(() => {});
31+
}
32+
};
33+
}

0 commit comments

Comments
 (0)