Plugins are not informed of shutdown and can leave resources dangling (e.g. typescript plugin)

[jnicklas]

• Rollup Version: 2.29.0, master
• Operating System (or Browser): macOS
• Node Version (if applicable): 12.10.0
• Link to reproduction (IMPORTANT, read below): https://github.com/jnicklas/rollup-cleanup-demo

Expected Behavior

When calling close() on a watcher created via watch(), it should also tell all used plugins that shutdown is imminent so that those plugins can close any resources they have allocated. If close() is called on a watcher, and there are no other open resources, node should exit.

Actual Behavior

Plugins are never informed of shutdown, the PluginHooks do not include a shutdown/close hook, and so the plugins are completely unaware that shutdown has been initiated. Plugins which have allocated internal resources, like for example the watcher used by the TypeScript plugin are not cleaned up. If using the TypeScript plugin, the watcher it has created internally is never removed, and so it is left running. As a result, node does not exit and hangs forever.

This is the reproduction in the linked repo, this script does not exit after printing closing. If the TypeScript plugin is removed it exits as normal.

let watch = require('rollup').watch;
let typescript = require('@rollup/plugin-typescript')

let watcher = watch({
  input: "./test.ts",
  plugins: [
    typescript()
  ]
});

console.log("running");
setTimeout(() => {
  console.log("closing");
  watcher.close()
}, 2000);

[shellscape]

@lukastaegert should we move this one to the main repo?

[lukastaegert]

Makes sense.

[lukastaegert]

So it sounds like we need a new plugin hook here, or extend the watchChange hook so that e.g. watchChange(null) means the watcher was closed. Opinions and further suggestions welcome.

[Amareis]

I think it should be extended even more - to support watcher hooks generally (and watch-only plugins, may be?..)

Currently I trying to make true incremental building for rollup (with preserveModules and disabled treeshaking, at least). It seems what such plugin can be made purely in user-land - but only if it will know which files is triggers rebuild.

So, basically, for watcher plugin we should just add hooks in public methods of Task - constructor (watchStart?), invalidate, and close. Additionally, pass to plugins BUILD_START/BUILD_END - if plugins need to modify RollupBuild before Task can handle it - and add BUILD_ERROR event for complete feature set.

WatchPluginContext can just give possibility to watch/unwatch files for first version.

It can be done in 50 lines of code, I think. @lukastaegert how you about to accept such PR?

[Amareis]

Simple config example for incremental plugin:

import incremental from 'rollup-plugin-incremental'

const inc = incremental({})
export default {
	input: 'src/main.js',
	treeshake: false,
	output: {
		dir: 'dist',
		format: 'es',
		preserveModules: true,
		preserveModulesRoot: 'src',
	},
	watcher: {
		plugins: [
			inc.getWatcherPlugin({}),
		]
	},
	plugins: [
		inc,
	]
}

[lukastaegert]

So, this is definitely a good idea, I wondered about these things myself, but there was always little time...

So, thoughts:

• I do not like the idea of having separate watcher plugins, but I do not think we need that, just more watch specific hooks
• I also do not think we need a watchStart hook because the buildStart hook should be good enough for that. You can find out if we are in watch mode via this.meta.watchMode
• A closeWatcher hook is definitely a good idea as that was the original request in this issue
• About invalidate could you elaborate here. We have a https://rollupjs.org/guide/en/#watchchange hook that is notified when a module was invalidated and gets the id of the invalidated module. But you are looking for a way to trigger manual invalidation?

[Amareis]

Oh, don't mind about watchChange. Yep, i think with it all is totally possible to implement without a watcher plugins. But watcher is coupled with a cache, and to modify it watcher events should be accessible inside of plugins.

[shellscape]

I can throw my support behind this as well

[lukastaegert]

Ok, but do you need any additional events besides closeWatcher that are not already covered? Or do you need more data? In any case, a PR for closeWatcher is definitely welcome. In any case, I hope we can stick with a single place to put plugins because it will make things easier for users. As the watcher is handed to Rollup, there is no problem triggering watcher events from within Rollup core.

[Amareis]

Ok, after implementing incremental plugin (https://github.com/mprt-org/rollup-plugin-incremental), i've see that

1. In Rollup types where is no this type for watchChange hook :)
2. There is no possibility to know which files already watched. For incremental building it's essential in order to add all watched files from previous builds - and not only which is really transformed to chunks, but all files which was be watched by plugins during resolve-load-etc hooks. We can extract transform dependencies, but that's all.
3. There is no possibility (without using fs) to know if file will be deleted or just updated. Again, for incremental building it's really matters, but don't sure what it's maters for another plugins.

So I think there should be such patch for types:

--- src/rollup/types.d.ts	(revision 7d42c4d43a0aaf2db73fef93c136c35af09cf19b)
+++ src/rollup/types.d.ts	(date 1603808293818)
@@ -195,6 +195,7 @@
 	getFileName: (fileReferenceId: string) => string;
 	getModuleIds: () => IterableIterator<string>;
 	getModuleInfo: GetModuleInfo;
+	getWatchFiles: () => string[];
 	/** @deprecated Use `this.resolve` instead */
 	isExternal: IsExternal;
 	/** @deprecated Use `this.getModuleIds` instead */
@@ -345,13 +346,14 @@
 export interface PluginHooks extends OutputPluginHooks {
 	buildEnd: (this: PluginContext, err?: Error) => Promise<void> | void;
 	buildStart: (this: PluginContext, options: NormalizedInputOptions) => Promise<void> | void;
+	closeWatcher: (this: PluginContext) => void;
 	load: LoadHook;
 	moduleParsed: ModuleParsedHook;
 	options: (this: MinimalPluginContext, options: InputOptions) => InputOptions | null | undefined;
 	resolveDynamicImport: ResolveDynamicImportHook;
 	resolveId: ResolveIdHook;
 	transform: TransformHook;
-	watchChange: (id: string) => void;
+	watchChange: (this: PluginContext, id: string, isDeleted: boolean) => void;
 }
 
 interface OutputPluginHooks {

[lukastaegert]

Makes sense. With regard to the watchChange addition, two things:

• Instead of an isDeleted flag, maybe rather have a string that contains the actual event: 'change', 'add' or 'unlink' because there are three possibilities at the moment
• Getting this information here without a breaking change is rather hard, though.

Because if you follow the code path, this is triggered by the change event of the watcher that again is triggered for all three events. And we cannot just add a parameter to the event as it already has a parameter, and you cannot have more than one. So the only solution I see here that would not be a breaking change would be to "deprecate" the old "change" event of the watcher (but still keep it around), and add a new event, e.g. "update" that has an object as parameter and is again triggered for all three situations.

So in general I would be very open to these changes, but there is some work involved (most notably at the moment, the watcher code immediately forgets about the type of event).