Directly restart Rollup when config file change is detected in watch …

…mode (#4344) * Improve test reliability * Increase timeout and repeat * Try again with a watcher * Try using fs.watchfile in the config * Wait a little after initial config write * Limit how long we wait at most * Immediately restart Rollup upon config file changes and change test to look for a message from the second config * See if we can trigger a stuck watcher by writing again * Extract retry helper * Clean up
rollup · Jan 14, 2022 · edca64e · edca64e
1 parent 9e947fc
commit edca64e
Show file tree

Hide file tree

Showing 10 changed files with 175 additions and 141 deletions.
diff --git a/cli/logging.ts b/cli/logging.ts
@@ -3,7 +3,7 @@ import { bold, cyan, dim, red } from '../src/utils/colors';
 import relativeId from '../src/utils/relativeId';
 
 // log to stderr to keep `rollup main.js > bundle.js` from breaking
-export const stderr = console.error.bind(console);
+export const stderr = (...args: unknown[]) => process.stderr.write(`${args.join('')}\n`);
 
 export function handleError(err: RollupError, recover = false): void {
 	let description = err.message || err;

diff --git a/cli/run/watch-cli.ts b/cli/run/watch-cli.ts
@@ -19,10 +19,9 @@ export async function watch(command: Record<string, any>): Promise<void> {
 	process.env.ROLLUP_WATCH = 'true';
 	const isTTY = process.stderr.isTTY;
 	const silent = command.silent;
-	let configs: MergedRollupOptions[];
-	let warnings: BatchWarnings;
 	let watcher: RollupWatcher;
 	let configWatcher: FSWatcher;
+	let resetScreen: (heading: string) => void;
 	const configFile = command.config ? getConfigPath(command.config) : null;
 
 	onExit(close);
@@ -33,11 +32,10 @@ export async function watch(command: Record<string, any>): Promise<void> {
 	}
 
 	async function loadConfigFromFileAndTrack(configFile: string): Promise<void> {
-		let reloadingConfig = false;
-		let aborted = false;
 		let configFileData: string | null = null;
+		let configFileRevision = 0;
 
-		configWatcher = chokidar.watch(configFile).on('change', () => reloadConfigFile());
+		configWatcher = chokidar.watch(configFile).on('change', reloadConfigFile);
 		await reloadConfigFile();
 
 		async function reloadConfigFile() {
@@ -46,29 +44,21 @@ export async function watch(command: Record<string, any>): Promise<void> {
 				if (newConfigFileData === configFileData) {
 					return;
 				}
-				if (reloadingConfig) {
-					aborted = true;
-					return;
-				}
+				configFileRevision++;
+				const currentConfigFileRevision = configFileRevision;
 				if (configFileData) {
 					stderr(`\nReloading updated config...`);
 				}
 				configFileData = newConfigFileData;
-				reloadingConfig = true;
-				({ options: configs, warnings } = await loadAndParseConfigFile(configFile, command));
-				reloadingConfig = false;
-				if (aborted) {
-					aborted = false;
-					reloadConfigFile();
-				} else {
-					if (watcher) {
-						watcher.close();
-					}
-					start(configs);
+				const { options, warnings } = await loadAndParseConfigFile(configFile, command);
+				if (currentConfigFileRevision !== configFileRevision) {
+					return;
 				}
+				if (watcher) {
+					watcher.close();
+				}
+				start(options, warnings);
 			} catch (err: any) {
-				configs = [];
-				reloadingConfig = false;
 				handleError(err, true);
 			}
 		}
@@ -77,13 +67,11 @@ export async function watch(command: Record<string, any>): Promise<void> {
 	if (configFile) {
 		await loadConfigFromFileAndTrack(configFile);
 	} else {
-		({ options: configs, warnings } = await loadConfigFromCommand(command));
-		start(configs);
+		const { options, warnings } = await loadConfigFromCommand(command);
+		start(options, warnings);
 	}
 
-	const resetScreen = getResetScreen(configs!, isTTY);
-
-	function start(configs: MergedRollupOptions[]): void {
+	function start(configs: MergedRollupOptions[], warnings: BatchWarnings): void {
 		try {
 			watcher = rollup.watch(configs as any);
 		} catch (err: any) {
@@ -99,6 +87,9 @@ export async function watch(command: Record<string, any>): Promise<void> {
 
 				case 'START':
 					if (!silent) {
+						if (!resetScreen) {
+							resetScreen = getResetScreen(configs, isTTY);
+						}
 						resetScreen(underline(`rollup v${rollup.VERSION}`));
 					}
 					break;

diff --git a/test/cli/index.js b/test/cli/index.js
@@ -35,113 +35,116 @@ function runTest(dir, config, pass) {
 			if (pass > 0) {
 				getFileNamesAndRemoveOutput(dir);
 			}
-			if (config.before) config.before();
-
 			const command = config.command.replace(
 				/(^| )rollup($| )/g,
 				`node ${path.resolve(__dirname, '../../dist/bin')}${path.sep}rollup `
 			);
 
-			const childProcess = exec(
-				command,
-				{
-					timeout: 40000,
-					env: { ...process.env, FORCE_COLOR: '0', ...config.env }
-				},
-				(err, code, stderr) => {
-					if (config.after) config.after(err, code, stderr);
-					if (err && !err.killed) {
-						if (config.error) {
-							const shouldContinue = config.error(err);
+			Promise.resolve(config.before && config.before()).then(() => {
+				const childProcess = exec(
+					command,
+					{
+						timeout: 40000,
+						env: { ...process.env, FORCE_COLOR: '0', ...config.env }
+					},
+					(err, code, stderr) => {
+						if (config.after) config.after(err, code, stderr);
+						if (err && !err.killed) {
+							if (config.error) {
+								const shouldContinue = config.error(err);
+								if (!shouldContinue) return done();
+							} else {
+								throw err;
+							}
+						}
+
+						if ('stderr' in config) {
+							const shouldContinue = config.stderr(stderr);
 							if (!shouldContinue) return done();
-						} else {
-							throw err;
+						} else if (stderr) {
+							console.error(stderr);
 						}
-					}
 
-					if ('stderr' in config) {
-						const shouldContinue = config.stderr(stderr);
-						if (!shouldContinue) return done();
-					} else if (stderr) {
-						console.error(stderr);
-					}
+						let unintendedError;
 
-					let unintendedError;
+						if (config.execute) {
+							try {
+								const fn = new Function('require', 'module', 'exports', 'assert', code);
+								const module = {
+									exports: {}
+								};
+								fn(require, module, module.exports, assert);
 
-					if (config.execute) {
-						try {
-							const fn = new Function('require', 'module', 'exports', 'assert', code);
-							const module = {
-								exports: {}
-							};
-							fn(require, module, module.exports, assert);
+								if (config.error) {
+									unintendedError = new Error('Expected an error while executing output');
+								}
 
-							if (config.error) {
-								unintendedError = new Error('Expected an error while executing output');
+								if (config.exports) {
+									config.exports(module.exports);
+								}
+							} catch (err) {
+								if (config.error) {
+									config.error(err);
+								} else {
+									unintendedError = err;
+								}
 							}
 
-							if (config.exports) {
-								config.exports(module.exports);
+							if (config.show || unintendedError) {
+								console.log(code + '\n\n\n');
 							}
-						} catch (err) {
-							if (config.error) {
-								config.error(err);
-							} else {
-								unintendedError = err;
-							}
-						}
 
-						if (config.show || unintendedError) {
-							console.log(code + '\n\n\n');
-						}
+							if (config.solo) console.groupEnd();
 
-						if (config.solo) console.groupEnd();
-
-						unintendedError ? done(unintendedError) : done();
-					} else if (config.result) {
-						try {
-							config.result(code);
-							done();
-						} catch (err) {
-							done(err);
-						}
-					} else if (config.test) {
-						try {
-							config.test();
-							done();
-						} catch (err) {
-							done(err);
-						}
-					} else if (sander.existsSync('_expected') && sander.statSync('_expected').isDirectory()) {
-						try {
-							assertDirectoriesAreEqual('_actual', '_expected');
-							done();
-						} catch (err) {
-							done(err);
-						}
-					} else {
-						const expected = sander.readFileSync('_expected.js').toString();
-						try {
-							assert.equal(normaliseOutput(code), normaliseOutput(expected));
-							done();
-						} catch (err) {
-							done(err);
+							unintendedError ? done(unintendedError) : done();
+						} else if (config.result) {
+							try {
+								config.result(code);
+								done();
+							} catch (err) {
+								done(err);
+							}
+						} else if (config.test) {
+							try {
+								config.test();
+								done();
+							} catch (err) {
+								done(err);
+							}
+						} else if (
+							sander.existsSync('_expected') &&
+							sander.statSync('_expected').isDirectory()
+						) {
+							try {
+								assertDirectoriesAreEqual('_actual', '_expected');
+								done();
+							} catch (err) {
+								done(err);
+							}
+						} else {
+							const expected = sander.readFileSync('_expected.js').toString();
+							try {
+								assert.equal(normaliseOutput(code), normaliseOutput(expected));
+								done();
+							} catch (err) {
+								done(err);
+							}
 						}
 					}
-				}
-			);
+				);
 
-			childProcess.stderr.on('data', async data => {
-				if (config.abortOnStderr) {
-					try {
-						if (await config.abortOnStderr(data)) {
+				childProcess.stderr.on('data', async data => {
+					if (config.abortOnStderr) {
+						try {
+							if (await config.abortOnStderr(data)) {
+								childProcess.kill('SIGTERM');
+							}
+						} catch (err) {
 							childProcess.kill('SIGTERM');
+							done(err);
 						}
-					} catch (err) {
-						childProcess.kill('SIGTERM');
-						done(err);
 					}
-				}
+				});
 			});
 		}
 	).timeout(50000);

diff --git a/test/cli/samples/wait-for-bundle-input/_config.js b/test/cli/samples/wait-for-bundle-input/_config.js
@@ -18,5 +18,7 @@ module.exports = {
 			// wait longer than one polling interval
 			setTimeout(() => atomicWriteFileSync(mainFile, 'export default 42;'), 600);
 		}
+		// We wait for a regular abort as we do not watch
+		return false;
 	}
 };
diff --git a/test/cli/samples/watch/no-config-file/_config.js b/test/cli/samples/watch/no-config-file/_config.js
@@ -1,8 +1,10 @@
+const path = require('path');
+
 module.exports = {
 	description: 'watches without a config file',
 	command: 'rollup main.js --watch --format es --file _actual/main.js',
 	abortOnStderr(data) {
-		if (data.includes('created _actual/main.js')) {
+		if (data.includes(`created _actual${path.sep}main.js`)) {
 			return true;
 		}
 	}

diff --git a/test/cli/samples/watch/node-config-file/_config.js b/test/cli/samples/watch/node-config-file/_config.js
@@ -1,8 +1,10 @@
+const path = require('path');
+
 module.exports = {
 	description: 'watches using a node_modules config files',
 	command: 'rollup --watch --config node:custom',
 	abortOnStderr(data) {
-		if (data.includes('created _actual/main.js')) {
+		if (data.includes(`created _actual${path.sep}main.js`)) {
 			return true;
 		}
 	}