From edca64e2aa09c4728d848888b72d00577a37e7a1 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Fri, 14 Jan 2022 14:08:11 +0100 Subject: [PATCH] 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 --- cli/logging.ts | 2 +- cli/run/watch-cli.ts | 45 ++--- test/cli/index.js | 173 +++++++++--------- .../samples/wait-for-bundle-input/_config.js | 2 + .../samples/watch/no-config-file/_config.js | 4 +- .../samples/watch/node-config-file/_config.js | 4 +- .../watch-config-early-update/_config.js | 58 +++--- .../watch/watch-config-error/_config.js | 2 +- .../watch/watch-config-no-update/_config.js | 5 +- test/utils.js | 21 +++ 10 files changed, 175 insertions(+), 141 deletions(-) diff --git a/cli/logging.ts b/cli/logging.ts index 959eae94071..59b4ec94630 100644 --- 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 index 9c83fc8265e..3e8e619a961 100644 --- a/cli/run/watch-cli.ts +++ b/cli/run/watch-cli.ts @@ -19,10 +19,9 @@ export async function watch(command: Record): Promise { 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): Promise { } async function loadConfigFromFileAndTrack(configFile: string): Promise { - 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): Promise { 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): Promise { 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): Promise { 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 index 7f747a34024..267b0b27e2d 100644 --- 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 index 64f197395b4..e27820f5bb6 100644 --- 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 index 7c7bb86caa3..af24c566137 100644 --- 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 index 10957e1893b..1181c599571 100644 --- 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; } } diff --git a/test/cli/samples/watch/watch-config-early-update/_config.js b/test/cli/samples/watch/watch-config-early-update/_config.js index 02e3884aa4d..031d5d38d14 100644 --- a/test/cli/samples/watch/watch-config-early-update/_config.js +++ b/test/cli/samples/watch/watch-config-early-update/_config.js @@ -1,33 +1,45 @@ const fs = require('fs'); const path = require('path'); -const { writeAndSync } = require('../../../../utils'); +const { writeAndSync, writeAndRetry } = require('../../../../utils'); -let configFile; +const configFile = path.join(__dirname, 'rollup.config.js'); +let stopUpdate; module.exports = { description: 'immediately reloads the config file if a change happens while it is parsed', command: 'rollup -cw', before() { - // This test writes a config file that prints a message to stderr but delays resolving to a - // config. The stderr message is observed by the parent process and triggers overwriting the - // config file. That way, we simulate a complicated config file being changed while it is parsed. - configFile = path.resolve(__dirname, 'rollup.config.js'); - fs.writeFileSync( + // This test writes a config file that prints a message to stderr which signals to the test that + // the config files has been parsed, at which point the test replaces the config file. The + // initial file returns a Promise that only resolves once the second config file has been + // parsed. To do that, the first config hooks into process.stderr and looks for a log from the + // second config. + // That way, we simulate a complicated config file being changed while it is parsed. + fs.mkdirSync(path.join(__dirname, '_actual')); + writeAndSync( configFile, ` - console.error('initial'); + import { Writable } from 'stream'; + process.stderr.write('initial\\n'); + const processStderr = process.stderr; export default new Promise(resolve => { - setTimeout( - () => - resolve({ - input: 'main.js', - output: { - file: '_actual/output1.js', - format: 'es' - } - }), - 2000 - ); + delete process.stderr; + process.stderr = new Writable({ + write(chunk, encoding, next) { + processStderr.write(chunk, encoding, next); + if (chunk.toString() === 'updated\\n') { + process.stderr.end(); + process.stderr = processStderr; + resolve({ + input: 'main.js', + output: { + file: '_actual/output1.js', + format: 'es' + } + }) + } + }, + }); });` ); }, @@ -36,7 +48,7 @@ module.exports = { }, abortOnStderr(data) { if (data === 'initial\n') { - writeAndSync( + stopUpdate = writeAndRetry( configFile, ` console.error('updated'); @@ -46,13 +58,13 @@ module.exports = { file: '_actual/output2.js', format: "es" } - }; - ` + };` ); return false; } if (data.includes(`created _actual${path.sep}output2.js`)) { - return new Promise(resolve => setTimeout(() => resolve(true), 600)); + stopUpdate(); + return true; } } }; diff --git a/test/cli/samples/watch/watch-config-error/_config.js b/test/cli/samples/watch/watch-config-error/_config.js index 400b1953b88..b313c17851f 100644 --- a/test/cli/samples/watch/watch-config-error/_config.js +++ b/test/cli/samples/watch/watch-config-error/_config.js @@ -48,7 +48,7 @@ module.exports = { return false; } if (data.includes(`created _actual${path.sep}main2.js`)) { - return new Promise(resolve => setTimeout(() => resolve(true), 600)); + return true; } } }; diff --git a/test/cli/samples/watch/watch-config-no-update/_config.js b/test/cli/samples/watch/watch-config-no-update/_config.js index 7bc29a79ab1..a90cb10e14a 100644 --- a/test/cli/samples/watch/watch-config-no-update/_config.js +++ b/test/cli/samples/watch/watch-config-no-update/_config.js @@ -23,9 +23,10 @@ module.exports = { fs.unlinkSync(configFile); }, abortOnStderr(data) { - if (data.includes('created _actual/main.js')) { + if (data.includes(`created _actual${path.sep}main.js`)) { atomicWriteFileSync(configFile, configContent); - return new Promise(resolve => setTimeout(() => resolve(true), 500)); + // wait some time for the watcher to trigger + return new Promise(resolve => setTimeout(() => resolve(true), 600)); } }, stderr(stderr) { diff --git a/test/utils.js b/test/utils.js index 7bd767d5751..7d391538477 100644 --- a/test/utils.js +++ b/test/utils.js @@ -18,6 +18,7 @@ exports.assertIncludes = assertIncludes; exports.atomicWriteFileSync = atomicWriteFileSync; exports.writeAndSync = writeAndSync; exports.getFileNamesAndRemoveOutput = getFileNamesAndRemoveOutput; +exports.writeAndRetry = writeAndRetry; function normaliseError(error) { delete error.stack; @@ -241,3 +242,23 @@ function writeAndSync(filePath, contents) { fs.fsyncSync(file); fs.closeSync(file); } + +// Sometimes, watchers on MacOS do not seem to fire. In those cases, it helps +// to write the same content again. This function returns a callback to stop +// further updates. +function writeAndRetry(filePath, contents) { + let retries = 0; + let updateRetryTimeout; + + const writeFile = () => { + if (retries > 0) { + console.error(`RETRIED writeFile (${retries})`); + } + retries++; + atomicWriteFileSync(filePath, contents); + updateRetryTimeout = setTimeout(writeFile, 1000); + }; + + writeFile(); + return () => clearTimeout(updateRetryTimeout); +}