Skip to content

Commit

Permalink
Do not abort watch mode on errors in watchChange (#4427)
Browse files Browse the repository at this point in the history
* Recovers from errors in the watchChange hook

* Also emit END event

* Fix direct npm install

* Await watchChange and closeWatcher hooks

* Extract emitter and improve coverage

* Update docs
  • Loading branch information
lukastaegert committed Mar 7, 2022
1 parent 6881753 commit cf75d71
Show file tree
Hide file tree
Showing 12 changed files with 249 additions and 127 deletions.
6 changes: 3 additions & 3 deletions cli/run/watch-cli.ts
Expand Up @@ -56,7 +56,7 @@ export async function watch(command: Record<string, any>): Promise<void> {
return;
}
if (watcher) {
watcher.close();
await watcher.close();
}
start(options, warnings);
} catch (err: any) {
Expand Down Expand Up @@ -136,12 +136,12 @@ export async function watch(command: Record<string, any>): Promise<void> {
});
}

function close(code: number | null): void {
async function close(code: number | null): Promise<void> {
process.removeListener('uncaughtException', close);
// removing a non-existent listener is a no-op
process.stdin.removeListener('end', close);

if (watcher) watcher.close();
if (watcher) await watcher.close();
if (configWatcher) configWatcher.close();

if (code) {
Expand Down
14 changes: 7 additions & 7 deletions docs/05-plugin-development.md
Expand Up @@ -50,7 +50,7 @@ export default ({

- Plugins should have a clear name with `rollup-plugin-` prefix.
- Include `rollup-plugin` keyword in `package.json`.
- Plugins should be tested. We recommend [mocha](https://github.com/mochajs/mocha) or [ava](https://github.com/avajs/ava) which support promises out of the box.
- Plugins should be tested. We recommend [mocha](https://github.com/mochajs/mocha) or [ava](https://github.com/avajs/ava) which support Promises out of the box.
- Use asynchronous methods when it is possible.
- Document your plugin in English.
- Make sure your plugin outputs correct source mappings if appropriate.
Expand All @@ -68,7 +68,7 @@ The name of the plugin, for use in error messages and warnings.

To interact with the build process, your plugin object includes 'hooks'. Hooks are functions which are called at various stages of the build. Hooks can affect how a build is run, provide information about a build, or modify a build once complete. There are different kinds of hooks:

- `async`: The hook may also return a promise resolving to the same type of value; otherwise, the hook is marked as `sync`.
- `async`: The hook may also return a Promise resolving to the same type of value; otherwise, the hook is marked as `sync`.
- `first`: If several plugins implement this hook, the hooks are run sequentially until a hook returns a value other than `null` or `undefined`.
- `sequential`: If several plugins implement this hook, all of them will be run in the specified plugin order. If a hook is async, subsequent hooks of this kind will wait until the current hook is resolved.
- `parallel`: If several plugins implement this hook, all of them will be run in the specified plugin order. If a hook is async, subsequent hooks of this kind will be run in parallel and not wait for the current hook.
Expand Down Expand Up @@ -96,9 +96,9 @@ Called on each `rollup.rollup` build. This is the recommended hook to use when y

#### `closeWatcher`

**Type:** `() => void`<br> **Kind:** `sync, sequential`<br> **Previous/Next Hook:** This hook can be triggered at any time both during the build and the output generation phases. If that is the case, the current build will still proceed but no new [`watchChange`](guide/en/#watchchange) events will be triggered ever.
**Type:** `() => void`<br> **Kind:** `async, parallel`<br> **Previous/Next Hook:** This hook can be triggered at any time both during the build and the output generation phases. If that is the case, the current build will still proceed but no new [`watchChange`](guide/en/#watchchange) events will be triggered ever.

Notifies a plugin when watcher process closes and all open resources should be closed too. This hook cannot be used by output plugins.
Notifies a plugin when the watcher process will close so that all open resources can be closed too. If a Promise is returned, Rollup will wait for the Promise to resolve before closing the process. This hook cannot be used by output plugins.

#### `load`

Expand Down Expand Up @@ -302,9 +302,9 @@ You can use [`this.getModuleInfo`](guide/en/#thisgetmoduleinfo) to find out the

#### `watchChange`

**Type:** `watchChange: (id: string, change: {event: 'create' | 'update' | 'delete'}) => void`<br> **Kind:** `sync, sequential`<br> **Previous/Next Hook:** This hook can be triggered at any time both during the build and the output generation phases. If that is the case, the current build will still proceed but a new build will be scheduled to start once the current build has completed, starting again with [`options`](guide/en/#options).
**Type:** `watchChange: (id: string, change: {event: 'create' | 'update' | 'delete'}) => void`<br> **Kind:** `async, parallel`<br> **Previous/Next Hook:** This hook can be triggered at any time both during the build and the output generation phases. If that is the case, the current build will still proceed but a new build will be scheduled to start once the current build has completed, starting again with [`options`](guide/en/#options).

Notifies a plugin whenever rollup has detected a change to a monitored file in `--watch` mode. This hook cannot be used by output plugins. Second argument contains additional details of change event.
Notifies a plugin whenever rollup has detected a change to a monitored file in `--watch` mode. If a Promise is returned, Rollup will wait for the Promise to resolve before scheduling another build. This hook cannot be used by output plugins. The second argument contains additional details of the change event.

### Output Generation Hooks

Expand Down Expand Up @@ -770,7 +770,7 @@ Loads and parses the module corresponding to the given id, attaching additional
This allows you to inspect the final content of modules before deciding how to resolve them in the [`resolveId`](guide/en/#resolveid) hook and e.g. resolve to a proxy module instead. If the module becomes part of the graph later, there is no additional overhead from using this context function as the module will not be parsed again. The signature allows you to directly pass the return value of [`this.resolve`](guide/en/#thisresolve) to this function as long as it is neither `null` nor external.
The returned promise will resolve once the module has been fully transformed and parsed but before any imports have been resolved. That means that the resulting `ModuleInfo` will have empty `importedIds`, `dynamicallyImportedIds`, `importedIdResolutions` and `dynamicallyImportedIdResolutions`. This helps to avoid deadlock situations when awaiting `this.load` in a `resolveId` hook. If you are interested in `importedIds` and `dynamicallyImportedIds`, you can either implement a `moduleParsed` hook or pass the `resolveDependencies` flag, which will make the promise returned by `this.load` wait until all dependency ids have been resolved.
The returned Promise will resolve once the module has been fully transformed and parsed but before any imports have been resolved. That means that the resulting `ModuleInfo` will have empty `importedIds`, `dynamicallyImportedIds`, `importedIdResolutions` and `dynamicallyImportedIdResolutions`. This helps to avoid deadlock situations when awaiting `this.load` in a `resolveId` hook. If you are interested in `importedIds` and `dynamicallyImportedIds`, you can either implement a `moduleParsed` hook or pass the `resolveDependencies` flag, which will make the Promise returned by `this.load` wait until all dependency ids have been resolved.
Note that with regard to the `moduleSideEffects`, `syntheticNamedExports` and `meta` options, the same restrictions apply as for the `resolveId` hook: Their values only have an effect if the module has not been loaded yet. Thus, it is very important to use `this.resolve` first to find out if any plugins want to set special values for these options in their `resolveId` hook, and pass these options on to `this.load` if appropriate. The example below showcases how this can be handled to add a proxy module for modules containing a special code comment. Note the special handling for re-exporting the default export:
Expand Down
4 changes: 2 additions & 2 deletions docs/build-hooks.mmd
Expand Up @@ -32,10 +32,10 @@ flowchart TB
transform("transform"):::hook-sequential
click transform "/guide/en/#transform" _parent

watchchange("watchChange"):::hook-sequential-sync
watchchange("watchChange"):::hook-parallel
click watchchange "/guide/en/#watchchange" _parent

closewatcher("closeWatcher"):::hook-sequential-sync
closewatcher("closeWatcher"):::hook-parallel
click closewatcher "/guide/en/#closewatcher" _parent

options
Expand Down
8 changes: 3 additions & 5 deletions package.json
Expand Up @@ -9,7 +9,7 @@
"rollup": "dist/bin/rollup"
},
"scripts": {
"build": "shx rm -rf dist && git rev-parse HEAD > .commithash && rollup --config rollup.config.ts --configPlugin typescript && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
"build": "shx rm -rf dist && node scripts/update-git-commit.js && rollup --config rollup.config.ts --configPlugin typescript && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
"build:cjs": "shx rm -rf dist && rollup --config rollup.config.ts --configPlugin typescript --configTest && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
"build:bootstrap": "node dist/bin/rollup --config rollup.config.ts --configPlugin typescript && shx cp src/rollup/types.d.ts dist/rollup.d.ts && shx chmod a+x dist/bin/rollup",
"ci:lint": "npm run lint:nofix",
Expand All @@ -22,10 +22,9 @@
"perf": "npm run build:cjs && node --expose-gc scripts/perf.js",
"perf:debug": "node --inspect-brk scripts/perf-debug.js",
"perf:init": "node scripts/perf-init.js",
"postinstall": "husky install",
"postpublish": "pinst --enable && git push && git push --tags",
"postpublish": "git push && git push --tags",
"prepare": "husky install && npm run build",
"prepublishOnly": "pinst --disable && npm ci && npm run lint:nofix && npm run security && npm run build:bootstrap && npm run test:all",
"prepublishOnly": "npm ci && npm run lint:nofix && npm run security && npm run build:bootstrap && npm run test:all",
"security": "npm audit",
"test": "npm run build && npm run test:all",
"test:cjs": "npm run build:cjs && npm run test:only",
Expand Down Expand Up @@ -97,7 +96,6 @@
"magic-string": "^0.25.7",
"mocha": "^9.2.1",
"nyc": "^15.1.0",
"pinst": "^3.0.0",
"prettier": "^2.5.1",
"pretty-bytes": "^5.6.0",
"pretty-ms": "^7.0.1",
Expand Down
12 changes: 12 additions & 0 deletions scripts/update-git-commit.js
@@ -0,0 +1,12 @@
const { execSync } = require('child_process');
const { writeFileSync } = require('fs');
const { join } = require('path');

let revision;
try {
revision = execSync('git rev-parse HEAD').toString().trim();
} catch (e) {
console.warn('Could not determine git commit when building Rollup.');
revision = '(could not be determined)';
}
writeFileSync(join(__dirname, '../.commithash'), revision);
14 changes: 5 additions & 9 deletions src/Graph.ts
Expand Up @@ -78,15 +78,11 @@ export default class Graph {

if (watcher) {
this.watchMode = true;
const handleChange: WatchChangeHook = (...args) =>
this.pluginDriver.hookSeqSync('watchChange', args);
const handleClose = () => this.pluginDriver.hookSeqSync('closeWatcher', []);
watcher.on('change', handleChange);
watcher.on('close', handleClose);
watcher.once('restart', () => {
watcher.removeListener('change', handleChange);
watcher.removeListener('close', handleClose);
});
const handleChange = (...args: Parameters<WatchChangeHook>) =>
this.pluginDriver.hookParallel('watchChange', args);
const handleClose = () => this.pluginDriver.hookParallel('closeWatcher', []);
watcher.onCurrentAwaited('change', handleChange);
watcher.onCurrentAwaited('close', handleClose);
}
this.pluginDriver = new PluginDriver(this, options, options.plugins, this.pluginCache);
this.acornParser = acorn.Parser.extend(...(options.acornInjectPlugins as any));
Expand Down
49 changes: 33 additions & 16 deletions src/rollup/types.d.ts
Expand Up @@ -344,7 +344,7 @@ export type WatchChangeHook = (
this: PluginContext,
id: string,
change: { event: ChangeEvent }
) => void;
) => Promise<void> | void;

/**
* use this type for plugin annotation
Expand Down Expand Up @@ -375,7 +375,7 @@ export interface PluginHooks extends OutputPluginHooks {
buildEnd: (this: PluginContext, err?: Error) => Promise<void> | void;
buildStart: (this: PluginContext, options: NormalizedInputOptions) => Promise<void> | void;
closeBundle: (this: PluginContext) => Promise<void> | void;
closeWatcher: (this: PluginContext) => void;
closeWatcher: (this: PluginContext) => Promise<void> | void;
load: LoadHook;
moduleParsed: ModuleParsedHook;
options: (
Expand Down Expand Up @@ -440,7 +440,9 @@ export type AsyncPluginHooks =
| 'shouldTransformCachedModule'
| 'transform'
| 'writeBundle'
| 'closeBundle';
| 'closeBundle'
| 'closeWatcher'
| 'watchChange';

export type PluginValueHooks = 'banner' | 'footer' | 'intro' | 'outro';

Expand All @@ -458,13 +460,11 @@ export type FirstPluginHooks =

export type SequentialPluginHooks =
| 'augmentChunkHash'
| 'closeWatcher'
| 'generateBundle'
| 'options'
| 'outputOptions'
| 'renderChunk'
| 'transform'
| 'watchChange';
| 'transform';

export type ParallelPluginHooks =
| 'banner'
Expand All @@ -477,7 +477,9 @@ export type ParallelPluginHooks =
| 'renderError'
| 'renderStart'
| 'writeBundle'
| 'closeBundle';
| 'closeBundle'
| 'closeWatcher'
| 'watchChange';

interface OutputPluginValueHooks {
banner: AddonHook;
Expand Down Expand Up @@ -898,6 +900,24 @@ interface TypedEventEmitter<T extends { [event: string]: (...args: any) => any }
setMaxListeners(n: number): this;
}

export interface RollupAwaitingEmitter<T extends { [event: string]: (...args: any) => any }>
extends TypedEventEmitter<T> {
close(): Promise<void>;
emitAndAwait<K extends keyof T>(event: K, ...args: Parameters<T[K]>): Promise<ReturnType<T[K]>[]>;
/**
* Registers an event listener that will be awaited before Rollup continues
* for events emitted via emitAndAwait. All listeners will be awaited in
* parallel while rejections are tracked via Promise.all.
* Listeners are removed automatically when removeAwaited is called, which
* happens automatically after each run.
*/
onCurrentAwaited<K extends keyof T>(
event: K,
listener: (...args: Parameters<T[K]>) => Promise<ReturnType<T[K]>>
): this;
removeAwaited(): this;
}

export type RollupWatcherEvent =
| { code: 'START' }
| { code: 'BUNDLE_START'; input?: InputOption; output: readonly string[] }
Expand All @@ -911,15 +931,12 @@ export type RollupWatcherEvent =
| { code: 'END' }
| { code: 'ERROR'; error: RollupError; result: RollupBuild | null };

export interface RollupWatcher
extends TypedEventEmitter<{
change: (id: string, change: { event: ChangeEvent }) => void;
close: () => void;
event: (event: RollupWatcherEvent) => void;
restart: () => void;
}> {
close(): void;
}
export type RollupWatcher = RollupAwaitingEmitter<{
change: (id: string, change: { event: ChangeEvent }) => void;
close: () => void;
event: (event: RollupWatcherEvent) => void;
restart: () => void;
}>;

export function watch(config: RollupWatchOptions | RollupWatchOptions[]): RollupWatcher;

Expand Down
11 changes: 0 additions & 11 deletions src/utils/PluginDriver.ts
Expand Up @@ -270,17 +270,6 @@ export class PluginDriver {
return promise;
}

// chains synchronously, ignores returns
hookSeqSync<H extends SyncPluginHooks & SequentialPluginHooks>(
hookName: H,
args: Parameters<PluginHooks[H]>,
replaceContext?: ReplaceContext
): void {
for (const plugin of this.plugins) {
this.runHookSync(hookName, args, plugin, replaceContext);
}
}

/**
* Run an async plugin hook and return the result.
* @param hookName Name of the plugin hook. Must be either in `PluginHooks` or `OutputPluginValueHooks`.
Expand Down
48 changes: 48 additions & 0 deletions src/watch/WatchEmitter.ts
@@ -0,0 +1,48 @@
import { EventEmitter } from 'events';

type PromiseReturn<T extends (...args: any) => any> = (
...args: Parameters<T>
) => Promise<ReturnType<T>>;

export class WatchEmitter<
T extends { [event: string]: (...args: any) => any }
> extends EventEmitter {
private awaitedHandlers: {
[K in keyof T]?: PromiseReturn<T[K]>[];
} = Object.create(null);

constructor() {
super();
// Allows more than 10 bundles to be watched without
// showing the `MaxListenersExceededWarning` to the user.
this.setMaxListeners(Infinity);
}

// Will be overwritten by Rollup
async close(): Promise<void> {}

emitAndAwait<K extends keyof T>(
event: K,
...args: Parameters<T[K]>
): Promise<ReturnType<T[K]>[]> {
this.emit(event as string, ...(args as any[]));
return Promise.all(this.getHandlers(event).map(handler => handler(...args)));
}

onCurrentAwaited<K extends keyof T>(
event: K,
listener: (...args: Parameters<T[K]>) => Promise<ReturnType<T[K]>>
): this {
this.getHandlers(event).push(listener);
return this;
}

removeAwaited(): this {
this.awaitedHandlers = {};
return this;
}

private getHandlers<K extends keyof T>(event: K): PromiseReturn<T[K]>[] {
return this.awaitedHandlers[event] || (this.awaitedHandlers[event] = []);
}
}
13 changes: 1 addition & 12 deletions src/watch/watch-proxy.ts
@@ -1,21 +1,10 @@
import { EventEmitter } from 'events';
import type { RollupWatcher } from '../rollup/types';
import { ensureArray } from '../utils/ensureArray';
import { errInvalidOption, error } from '../utils/error';
import type { GenericConfigObject } from '../utils/options/options';
import { WatchEmitter } from './WatchEmitter';
import { loadFsEvents } from './fsevents-importer';

class WatchEmitter extends EventEmitter {
constructor() {
super();
// Allows more than 10 bundles to be watched without
// showing the `MaxListenersExceededWarning` to the user.
this.setMaxListeners(Infinity);
}

close() {}
}

export default function watch(configs: GenericConfigObject[] | GenericConfigObject): RollupWatcher {
const emitter = new WatchEmitter() as RollupWatcher;
const configArray = ensureArray(configs);
Expand Down
31 changes: 23 additions & 8 deletions src/watch/watch.ts
Expand Up @@ -57,12 +57,12 @@ export class Watcher {
process.nextTick(() => this.run());
}

close(): void {
async close(): Promise<void> {
if (this.buildTimeout) clearTimeout(this.buildTimeout);
for (const task of this.tasks) {
task.close();
}
this.emitter.emit('close');
await this.emitter.emitAndAwait('close');
this.emitter.removeAllListeners();
}

Expand All @@ -87,14 +87,29 @@ export class Watcher {

if (this.buildTimeout) clearTimeout(this.buildTimeout);

this.buildTimeout = setTimeout(() => {
this.buildTimeout = setTimeout(async () => {
this.buildTimeout = null;
for (const [id, event] of this.invalidatedIds) {
this.emitter.emit('change', id, { event });
try {
await Promise.all(
[...this.invalidatedIds].map(([id, event]) =>
this.emitter.emitAndAwait('change', id, { event })
)
);
this.invalidatedIds.clear();
this.emitter.emit('restart');
this.emitter.removeAwaited();
this.run();
} catch (error: any) {
this.invalidatedIds.clear();
this.emitter.emit('event', {
code: 'ERROR',
error,
result: null
});
this.emitter.emit('event', {
code: 'END'
});
}
this.invalidatedIds.clear();
this.emitter.emit('restart');
this.run();
}, this.buildDelay);
}

Expand Down

0 comments on commit cf75d71

Please sign in to comment.