Detect unfulfilled async hook actions and report error on exit #4320
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4320 +/- ##
=======================================
Coverage 98.44% 98.44%
=======================================
Files 205 206 +1
Lines 7322 7353 +31
Branches 2085 2089 +4
=======================================
+ Hits 7208 7239 +31
Misses 55 55
Partials 59 59
Continue to review full report at Codecov.
|
|
This PR was changed to move all the unfulfilled hook action error reporting into |
|
I added the unrelated flakey watch test workaround #4318 (comment) to this PR because github was experiencing load issues yesterday leading to that watch test failing almost every run on macos. FWIW, it does appear to work under load. That particular commit can be removed in favor of integrating it into #4318. |
src/utils/PluginDriver.ts
Outdated
| // provides uniqueness for unfulfilled hook actions and the dispatch order is useful for debugging | ||
| let actionCount = 0; | ||
|
|
||
| function formatAction(pluginName: string, hookName: string, args: Parameters<any>): string { |
There was a problem hiding this comment.
One thought here: Currently you are using a set of strings, which means those strings also need to be unique which means you need the actionCount etc.
Also, you are basically formatting the error that does not exist yet, which feels - slightly wrong to me. Especially as it separates the formatting logic from the reporting logic (ok, they are placed next to each other, but logically, there are executed at very different points).
Why not use objects (or tuples [plugin.name, hookName, args]) as actions instead? Then you would have even less overhead, but we could have as expensive stuff as we want in the error reporting. And most importantly, the error reporting would be separate.
src/utils/PluginDriver.ts
Outdated
| return action; | ||
| } | ||
|
|
||
| if (typeof process === 'object' && process && typeof process.on === 'function') { |
There was a problem hiding this comment.
I am not a fan of placing node-specific logic here, even if well guarded, but I know you tend to disagree. Still, as this will not work in the browser anyway, why not do something similar to what we do for fs and path: Put the logic into a separate file that is replaced with a no-op browser version for the browser build?
Then you do not need the checks, and we could even move out more logic. E.g. the file could export two functions "addUnresolvedAction(actionTuple)" and "resolveAction(actionTuple)" and everything else including the reporting would be a black box. Very likely, tree-shaking would then even remove the calls to these functions if they do not do anything in the browser version.
* 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.
|
The requested changes were implemented. Feel free to take over this PR for any additional fine tuning. Does the browser version of rollup support any sort of Plugin? If so, are there any browser Plugin tests? |
|
To answer my own question, the browser version of rollup does indeed support plugins, but the existing tests only exercise synchronous hook actions. Just as an experiment I altered a browser test to see if it will run an async hook - and it did: diff --git a/browser/hookActions.ts b/browser/hookActions.ts
index 015f6cc..b9e3bf4 100644
--- a/browser/hookActions.ts
+++ b/browser/hookActions.ts
@@ -1,3 +1,7 @@
-export function addUnresolvedAction(_actionTuple: [string, string, Parameters<any>]): void {}
+export function addUnresolvedAction(_actionTuple: [string, string, Parameters<any>]): void {
+ console.error('*** hookAction: addUnresolvedAction', _actionTuple[0], _actionTuple[1]);
+}
-export function resolveAction(_actionTuple: [string, string, Parameters<any>]): void {}
+export function resolveAction(_actionTuple: [string, string, Parameters<any>]): void {
+ console.error('*** hookAction: resolveAction', _actionTuple[0], _actionTuple[1]);
+}
diff --git a/test/browser/samples/basic/_config.js b/test/browser/samples/basic/_config.js
index a188163..16434f0 100644
--- a/test/browser/samples/basic/_config.js
+++ b/test/browser/samples/basic/_config.js
@@ -3,10 +3,19 @@ const { loader } = require('../../../utils.js');
module.exports = {
description: 'bundles files for the browser',
options: {
- plugins: loader({
- main: `import {foo} from 'dep';
+ plugins: [
+ loader({
+ main: `import {foo} from 'dep';
console.log(foo);`,
- dep: `export const foo = 42;`
- })
+ dep: `export const foo = 42;`
+ }), {
+ name: "async-browser-plugin",
+ transform(code, id) {
+ // this action will never resolve or reject
+ console.error("*** async-browser-plugin transform");
+ return new Promise(() => {});
+ },
+ }
+ ]
}
};The unfulfilled async transform hook action failed with a mocha test timeout as expected: I suppose one could add timeout logic into |
You answered the question yourself, but as a rule of thumb, the browser build supports everything the Node build does (it is more or less the same code after all), it is just missing the fallbacks for the load and resolveId hooks. |
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.
Negligible speed impact. Will not slow rollup operation or delay
the reporting of errors. The implementation does not use timeouts.
This PR contains:
Are tests included?
Breaking Changes?
List any relevant issue numbers:
Description
It was evident in the course of development of rollup/plugins#1038 that several users had a confusing user experience with plugin async hook bugs causing rollup to exit abruptly without reporting any warnings, errors, or a non-zero error code upon failure:
This PR addresses that rollup async plugin hook error reporting shortcoming that may occur in plugin development and plugin use in the wild.