Watch files onbuild, even if build fails (#3081)

* Watch files onbuild, even if build fails

* Add array, not graph, to error object

* Update error format for tests

* Update watch tests to throw ERRORs, not FATAL

* Add test for recovering from error on watch

* Fix eslint-utils security vulnerability

* More restrictive object testing, ts fixes

* Better type checking

* Better type checking

* Improve watch behaviour
* Do not add files to Graph.watchFiles after all files have been loaded
  but immediately when starting to load them so that the same watchFile
  logic can be used for successful and unsuccessful builds (and files
  added by plugins are not ignored on errors)
* Do not unwatch files that have been removed if an error occurred but
  only when a build has succeeded
* Do not re-watch files of the previous build when an error occurred as
  those are still being watched; just additionally watch any files
  attached to the error
* Make sure that watching a file automatically adds it to the list of
  "watched" files and only reset this list when clearing unneeded files
* Remove logic to filter for absolute ids for now to restore the old
  behaviour
* Replace some .forEach with for loops for slightly improved performance

* Add watchFiles to errors not only when an explicit file position is
given but for any error that occurs during the build phase when there
are already some watchFiles

* Only keep watching deleted files when using chokidar

* Improve coverage and remove unused edge cases

* Seems like watching missing files just does not work out reliably on all systems

* Remove unused check

* Add test for virtual files

src/Graph.ts

@@ -222,7 +222,6 @@ export default class Graph {
 			for (const module of this.moduleById.values()) {
 				if (module instanceof Module) {
 					this.modules.push(module);
-					this.watchFiles[module.id] = true;
 				} else {
 					this.externalModules.push(module);
 				}
@@ -331,7 +330,7 @@ export default class Graph {
 		return {
 			modules: this.modules.map(module => module.toJSON()),
 			plugins: this.pluginCache
-		} as any;
+		};
 	}
 
 	includeMarked(modules: Module[]) {

src/Module.ts

@@ -218,7 +218,7 @@ export default class Module {
 	private graph: Graph;
 	private magicString!: MagicString;
 	private namespaceVariable: NamespaceVariable = undefined as any;
-	private transformDependencies: string[] | null = null;
+	private transformDependencies: string[] = [];
 	private transitiveReexports?: string[];
 
 	constructor(graph: Graph, id: string, moduleSideEffects: boolean, isEntry: boolean) {
@@ -244,7 +244,6 @@ export default class Module {
 	error(props: RollupError, pos: number) {
 		if (pos !== undefined) {
 			props.pos = pos;
-
 			let location = locate(this.code, pos, { offsetLine: 1 });
 			try {
 				location = getOriginalLocation(this.sourcemapChain, location);
@@ -272,6 +271,8 @@ export default class Module {
 			props.frame = getCodeFrame(this.originalCode, location.line, location.column);
 		}
 
+		props.watchFiles = Object.keys(this.graph.watchFiles);
+
 		error(props);
 	}
 

src/ModuleLoader.ts

@@ -280,6 +280,7 @@ export class ModuleLoader {
 
 		const module: Module = new Module(this.graph, id, moduleSideEffects, isEntry);
 		this.modulesById.set(id, module);
+		this.graph.watchFiles[id] = true;
 		const manualChunkAlias = this.getManualChunk(id);
 		if (typeof manualChunkAlias === 'string') {
 			this.addModuleToManualChunk(manualChunkAlias, module);

src/rollup/types.d.ts

@@ -5,6 +5,7 @@ export const VERSION: string;
 
 export interface RollupError extends RollupLogProps {
 	stack?: string;
+	watchFiles?: string[];
 }
 
 export interface RollupWarning extends RollupLogProps {
@@ -105,7 +106,7 @@ export interface TransformModuleJSON {
 	originalSourcemap: ExistingDecodedSourceMap | null;
 	resolvedIds?: ResolvedIdMap;
 	sourcemapChain: DecodedSourceMapOrMissing[];
-	transformDependencies: string[] | null;
+	transformDependencies: string[];
 }
 
 export interface ModuleJSON extends TransformModuleJSON {
@@ -527,7 +528,7 @@ export interface SerializablePluginCache {
 }
 
 export interface RollupCache {
-	modules?: ModuleJSON[];
+	modules: ModuleJSON[];
 	plugins?: Record<string, SerializablePluginCache>;
 }
 

src/watch/fileWatchers.ts

@@ -53,16 +53,16 @@ export default class FileWatcher {
 				// can't watch files that don't exist (e.g. injected
 				// by plugins somehow)
 				return;
-			} else {
-				throw err;
 			}
+			throw err;
 		}
 
 		const handleWatchEvent = (event: string) => {
 			if (event === 'rename' || event === 'unlink') {
 				this.close();
 				group.delete(id);
 				this.trigger(id);
+				return;
 			} else {
 				let stats: fs.Stats;
 				try {
@@ -87,7 +87,7 @@ export default class FileWatcher {
 		group.set(id, this);
 	}
 
-	addTask(task: Task, isTransformDependency = false) {
+	addTask(task: Task, isTransformDependency: boolean) {
 		if (isTransformDependency) this.transformDependencyTasks.add(task);
 		else this.tasks.add(task);
 	}

src/watch/index.ts

@@ -5,10 +5,10 @@ import createFilter from 'rollup-pluginutils/src/createFilter';
 import rollup, { setWatcher } from '../rollup/index';
 import {
 	InputOptions,
-	ModuleJSON,
 	OutputOptions,
 	RollupBuild,
 	RollupCache,
+	RollupError,
 	RollupWatcher,
 	WatcherOptions
 } from '../rollup/types';
@@ -25,7 +25,6 @@ export class Watcher {
 	private invalidatedIds: Set<string> = new Set();
 	private rerun = false;
 	private running: boolean;
-	private succeeded = false;
 	private tasks: Task[];
 
 	constructor(configs: GenericConfigObject[] | GenericConfigObject) {
@@ -48,9 +47,9 @@ export class Watcher {
 
 	close() {
 		if (this.buildTimeout) clearTimeout(this.buildTimeout);
-		this.tasks.forEach(task => {
+		for (const task of this.tasks) {
 			task.close();
-		});
+		}
 
 		this.emitter.removeAllListeners();
 	}
@@ -72,7 +71,9 @@ export class Watcher {
 
 		this.buildTimeout = setTimeout(() => {
 			this.buildTimeout = null;
-			this.invalidatedIds.forEach(id => this.emit('change', id));
+			for (const id of this.invalidatedIds) {
+				this.emit('change', id);
+			}
 			this.invalidatedIds.clear();
 			this.emit('restart');
 			this.run();
@@ -90,7 +91,6 @@ export class Watcher {
 		for (const task of this.tasks) taskPromise = taskPromise.then(() => task.run());
 		return taskPromise
 			.then(() => {
-				this.succeeded = true;
 				this.running = false;
 
 				this.emit('event', {
@@ -100,7 +100,7 @@ export class Watcher {
 			.catch(error => {
 				this.running = false;
 				this.emit('event', {
-					code: this.succeeded ? 'ERROR' : 'FATAL',
+					code: 'ERROR',
 					error
 				});
 			})
@@ -114,7 +114,7 @@ export class Watcher {
 }
 
 export class Task {
-	cache: RollupCache;
+	cache: RollupCache = { modules: [] };
 	watchFiles: string[] = [];
 
 	private chokidarOptions: WatchOptions;
@@ -129,7 +129,6 @@ export class Task {
 	private watcher: Watcher;
 
 	constructor(watcher: Watcher, config: GenericConfigObject) {
-		this.cache = null as any;
 		this.watcher = watcher;
 
 		this.closed = false;
@@ -172,20 +171,19 @@ export class Task {
 
 	close() {
 		this.closed = true;
-		this.watched.forEach(id => {
+		for (const id of this.watched) {
 			deleteTask(id, this, this.chokidarOptionsHash);
-		});
+		}
 	}
 
 	invalidate(id: string, isTransformDependency: boolean) {
 		this.invalidated = true;
 		if (isTransformDependency) {
-			(this.cache.modules as ModuleJSON[]).forEach(module => {
-				if (!module.transformDependencies || module.transformDependencies.indexOf(id) === -1)
-					return;
+			for (const module of this.cache.modules) {
+				if (module.transformDependencies.indexOf(id) === -1) return;
 				// effective invalidation
 				module.originalCode = null as any;
-			});
+			}
 		}
 		this.watcher.invalidate(id);
 	}
@@ -211,27 +209,7 @@ export class Task {
 		return rollup(options)
 			.then(result => {
 				if (this.closed) return undefined as any;
-				const previouslyWatched = this.watched;
-				const watched = (this.watched = new Set());
-
-				this.cache = result.cache;
-				this.watchFiles = result.watchFiles;
-				for (const module of this.cache.modules as ModuleJSON[]) {
-					if (module.transformDependencies) {
-						module.transformDependencies.forEach(depId => {
-							watched.add(depId);
-							this.watchFile(depId, true);
-						});
-					}
-				}
-				for (const id of this.watchFiles) {
-					watched.add(id);
-					this.watchFile(id);
-				}
-				for (const id of previouslyWatched) {
-					if (!watched.has(id)) deleteTask(id, this, this.chokidarOptionsHash);
-				}
-
+				this.updateWatchedFiles(result);
 				return Promise.all(this.outputs.map(output => result.write(output))).then(() => result);
 			})
 			.then((result: RollupBuild) => {
@@ -243,31 +221,39 @@ export class Task {
 					result
 				});
 			})
-			.catch((error: Error) => {
+			.catch((error: RollupError) => {
 				if (this.closed) return;
 
-				if (this.cache) {
-					// this is necessary to ensure that any 'renamed' files
-					// continue to be watched following an error
-					if (this.cache.modules) {
-						this.cache.modules.forEach(module => {
-							if (module.transformDependencies) {
-								module.transformDependencies.forEach(depId => {
-									this.watchFile(depId, true);
-								});
-							}
-						});
-					}
-					this.watchFiles.forEach(id => {
+				if (Array.isArray(error.watchFiles)) {
+					for (const id of error.watchFiles) {
 						this.watchFile(id);
-					});
+					}
 				}
 				throw error;
 			});
 	}
 
-	watchFile(id: string, isTransformDependency = false) {
+	private updateWatchedFiles(result: RollupBuild) {
+		const previouslyWatched = this.watched;
+		this.watched = new Set();
+		this.watchFiles = result.watchFiles;
+		this.cache = result.cache;
+		for (const id of this.watchFiles) {
+			this.watchFile(id);
+		}
+		for (const module of this.cache.modules) {
+			for (const depId of module.transformDependencies) {
+				this.watchFile(depId, true);
+			}
+		}
+		for (const id of previouslyWatched) {
+			if (!this.watched.has(id)) deleteTask(id, this, this.chokidarOptionsHash);
+		}
+	}
+
+	private watchFile(id: string, isTransformDependency = false) {
 		if (!this.filter(id)) return;
+		this.watched.add(id);
 
 		if (this.outputFiles.some(file => file === id)) {
 			throw new Error('Cannot import the generated bundle');

test/function/samples/cannot-call-external-namespace/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'CANNOT_CALL_NAMESPACE',
 		message: `Cannot call a namespace ('foo')`,
 		pos: 28,
+		watchFiles: [path.resolve(__dirname, 'main.js')],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 2,

test/function/samples/cannot-call-internal-namespace/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'CANNOT_CALL_NAMESPACE',
 		message: `Cannot call a namespace ('foo')`,
 		pos: 33,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'foo.js')],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 2,

test/function/samples/default-not-reexported/_config.js

@@ -6,6 +6,11 @@ module.exports = {
 		code: 'MISSING_EXPORT',
 		message: `'default' is not exported by foo.js`,
 		pos: 7,
+		watchFiles: [
+			path.resolve(__dirname, 'main.js'),
+			path.resolve(__dirname, 'foo.js'),
+			path.resolve(__dirname, 'bar.js')
+		],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 1,

test/function/samples/double-default-export/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: `Duplicate export 'default'`,
 		pos: 25,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'foo.js')],
 		loc: {
 			file: path.resolve(__dirname, 'foo.js'),
 			line: 2,

test/function/samples/double-named-export/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: `Duplicate export 'foo'`,
 		pos: 38,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'foo.js')],
 		loc: {
 			file: path.resolve(__dirname, 'foo.js'),
 			line: 3,

test/function/samples/double-named-reexport/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: `Duplicate export 'foo'`,
 		pos: 38,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'foo.js')],
 		loc: {
 			file: path.resolve(__dirname, 'foo.js'),
 			line: 3,

test/function/samples/duplicate-import-fails/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: `Identifier 'a' has already been declared`,
 		pos: 36,
+		watchFiles: [path.resolve(__dirname, 'main.js')],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 2,

test/function/samples/duplicate-import-specifier-fails/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: `Identifier 'a' has already been declared`,
 		pos: 12,
+		watchFiles: [path.resolve(__dirname, 'main.js')],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 1,

test/function/samples/error-after-transform-should-throw-correct-location/_config.js

@@ -22,6 +22,7 @@ module.exports = {
 		code: 'MISSING_EXPORT',
 		message: `'default' is not exported by empty.js`,
 		pos: 44,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'empty.js')],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 1,

test/function/samples/error-parse-json/_config.js

@@ -7,6 +7,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: 'Unexpected token (Note that you need rollup-plugin-json to import JSON files)',
 		pos: 10,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'file.json')],
 		loc: {
 			file: path.resolve(__dirname, 'file.json'),
 			line: 2,

test/function/samples/error-parse-unknown-extension/_config.js

@@ -8,6 +8,7 @@ module.exports = {
 		message:
 			'Unexpected token (Note that you need plugins to import files that are not JavaScript)',
 		pos: 0,
+		watchFiles: [path.resolve(__dirname, 'main.js'), path.resolve(__dirname, 'file.css')],
 		loc: {
 			file: path.resolve(__dirname, 'file.css'),
 			line: 1,

test/function/samples/export-not-at-top-level-fails/_config.js

@@ -6,6 +6,7 @@ module.exports = {
 		code: 'PARSE_ERROR',
 		message: `'import' and 'export' may only appear at the top level`,
 		pos: 19,
+		watchFiles: [path.resolve(__dirname, 'main.js')],
 		loc: {
 			file: path.resolve(__dirname, 'main.js'),
 			line: 2,