From 8a83c8aa3588d97762e7529ce883427b23f898b1 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Tue, 15 Mar 2016 17:58:43 -0700 Subject: [PATCH 01/11] style mistakes in test --- test/test-all.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/test/test-all.js b/test/test-all.js index d349e25..c6d9469 100644 --- a/test/test-all.js +++ b/test/test-all.js @@ -36,7 +36,7 @@ describe('chokidar-cli', function() { .then(function() { done(); }); - }) + }); it('help should be succesful', function(done) { run('node index.js --help', {pipe: DEBUG_TESTS}) @@ -88,8 +88,8 @@ describe('chokidar-cli', function() { fs.writeFileSync(resolve('dir/subdir/c.less'), 'content'); setTimeout(function() { - assert(changeFileExists(), 'change file should exist') - }, TIMEOUT_CHANGE_DETECTED) + assert(changeFileExists(), 'change file should exist'); + }, TIMEOUT_CHANGE_DETECTED); }, TIMEOUT_WATCH_READY); }); @@ -110,7 +110,7 @@ describe('chokidar-cli', function() { .then(function() { var res = fs.readFileSync(resolve(CHANGE_FILE)).toString().trim(); assert.equal(res, 'change:dir/a.js', 'need event/path detail'); - done() + done(); }); }); }); From 95603d4ab9719118cf041c3da649ffd7f1debae2 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Mon, 21 Mar 2016 02:03:27 -0700 Subject: [PATCH 02/11] refactor run to return a child process, breaks throttle and debounce --- index.js | 54 ++++++++++++++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 16 deletions(-) diff --git a/index.js b/index.js index e638668..e2fa74d 100755 --- a/index.js +++ b/index.js @@ -4,7 +4,6 @@ var childProcess = require('child_process'); var Promise = require('bluebird'); var _ = require('lodash'); var chokidar = require('chokidar'); -var utils = require('./utils'); var EVENT_DESCRIPTIONS = { add: 'File added', @@ -14,6 +13,12 @@ var EVENT_DESCRIPTIONS = { change: 'File changed' }; +// Try to resolve path to shell. +// We assume that Windows provides COMSPEC env variable +// and other platforms provide SHELL env variable +var SHELL_PATH = process.env.SHELL || process.env.COMSPEC; +var EXECUTE_OPTION = process.env.COMSPEC !== undefined && process.env.SHELL === undefined ? '/c' : '-c'; + var defaultOpts = { debounce: 400, throttle: 0, @@ -25,7 +30,8 @@ var defaultOpts = { verbose: false, silent: false, initial: false, - command: null + command: null, + concurrent: false }; var VERSION = 'chokidar-cli: ' + require('./package.json').version + @@ -83,6 +89,11 @@ var argv = require('yargs') default: defaultOpts.initial, type: 'boolean' }) + .option('concurrent', { + describe: 'When set, command is not killed before invoking again', + default: defaultOpts.concurrent, + type: 'boolean' + }) .option('p', { alias: 'polling', describe: 'Whether to use fs.watchFile(backed by polling) instead of ' + @@ -134,15 +145,29 @@ function getUserOpts(argv) { return argv; } -// Estimates spent working hours based on commit dates function startWatching(opts) { var chokidarOpts = createChokidarOpts(opts); var watcher = chokidar.watch(opts.patterns, chokidarOpts); + var child; - var throttledRun = _.throttle(run, opts.throttle); - var debouncedRun = _.debounce(throttledRun, opts.debounce); + // var throttledRun = _.throttle(run, opts.throttle); + // var debouncedRun = _.debounce(throttledRun, opts.debounce); watcher.on('all', function(event, path) { var description = EVENT_DESCRIPTIONS[event] + ':'; + function executeCommand() { + if (child) child.removeAllListeners(); + child = run( + opts.command + .replace(/\{path\}/ig, path) + .replace(/\{event\}/ig, event) + ); + child.once('error', function(error) { + throw error; + }); + child.once('exit', function() { + child = undefined; + }); + } if (opts.verbose) { console.error(description, path); @@ -152,13 +177,14 @@ function startWatching(opts) { } } - // XXX: commands might be still run concurrently if (opts.command) { - debouncedRun( - opts.command - .replace(/\{path\}/ig, path) - .replace(/\{event\}/ig, event) - ); + // If a previous run of command created a child, and the concurrent option is not set, + // then we should kill that child process before running it again + if (child && !opts.concurrent) { + child.once('exit', executeCommand); + child.kill(); + } + setImmediate(executeCommand); } }); @@ -212,11 +238,7 @@ function _resolveIgnoreOpt(ignoreOpt) { } function run(cmd) { - return utils.run(cmd) - .catch(function(err) { - console.error('Error when executing', cmd); - console.error(err.stack); - }); + return childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, cmd]); } main(); From d4d74ec6b338457de331f5f2ac36a96228cf14be Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Mon, 21 Mar 2016 02:36:02 -0700 Subject: [PATCH 03/11] restore throttle and debounce functionality (needs tests) --- index.js | 27 ++++++++++++--------------- 1 file changed, 12 insertions(+), 15 deletions(-) diff --git a/index.js b/index.js index e2fa74d..60aa7f6 100755 --- a/index.js +++ b/index.js @@ -150,23 +150,17 @@ function startWatching(opts) { var watcher = chokidar.watch(opts.patterns, chokidarOpts); var child; - // var throttledRun = _.throttle(run, opts.throttle); - // var debouncedRun = _.debounce(throttledRun, opts.debounce); watcher.on('all', function(event, path) { var description = EVENT_DESCRIPTIONS[event] + ':'; function executeCommand() { - if (child) child.removeAllListeners(); - child = run( - opts.command - .replace(/\{path\}/ig, path) - .replace(/\{event\}/ig, event) - ); - child.once('error', function(error) { - throw error; - }); - child.once('exit', function() { - child = undefined; - }); + console.log('will run command'); + _.debounce(_.throttle(function() { + if (child) child.removeAllListeners(); + console.log('spawning new command process'); + child = childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event)]); + child.once('error', function(error) { throw error; }); + child.once('exit', function() { child = undefined; }); + }, opts.throttle), opts.debounce)(); } if (opts.verbose) { @@ -181,10 +175,13 @@ function startWatching(opts) { // If a previous run of command created a child, and the concurrent option is not set, // then we should kill that child process before running it again if (child && !opts.concurrent) { + console.log('run command after killing previous child'); child.once('exit', executeCommand); child.kill(); + } else { + console.log('run command'); + setImmediate(executeCommand); } - setImmediate(executeCommand); } }); From bfd0479f8049933e650a3bb377bbc3273309728b Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Mon, 21 Mar 2016 11:50:30 -0700 Subject: [PATCH 04/11] throttle fixes and test --- index.js | 20 ++++++++++---------- test/test-all.js | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/index.js b/index.js index 60aa7f6..5a56db1 100755 --- a/index.js +++ b/index.js @@ -149,18 +149,20 @@ function startWatching(opts) { var chokidarOpts = createChokidarOpts(opts); var watcher = chokidar.watch(opts.patterns, chokidarOpts); var child; + var execFn; watcher.on('all', function(event, path) { var description = EVENT_DESCRIPTIONS[event] + ':'; function executeCommand() { - console.log('will run command'); - _.debounce(_.throttle(function() { - if (child) child.removeAllListeners(); - console.log('spawning new command process'); - child = childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event)]); - child.once('error', function(error) { throw error; }); - child.once('exit', function() { child = undefined; }); - }, opts.throttle), opts.debounce)(); + if (!execFn) { + execFn = _.debounce(_.throttle(function() { + if (child) child.removeAllListeners(); + child = childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event)]); + child.once('error', function(error) { throw error; }); + child.once('exit', function() { child = undefined; }); + }, opts.throttle), opts.debounce); + } + execFn(); } if (opts.verbose) { @@ -175,11 +177,9 @@ function startWatching(opts) { // If a previous run of command created a child, and the concurrent option is not set, // then we should kill that child process before running it again if (child && !opts.concurrent) { - console.log('run command after killing previous child'); child.once('exit', executeCommand); child.kill(); } else { - console.log('run command'); setImmediate(executeCommand); } } diff --git a/test/test-all.js b/test/test-all.js index c6d9469..a3f86af 100644 --- a/test/test-all.js +++ b/test/test-all.js @@ -93,6 +93,45 @@ describe('chokidar-cli', function() { }, TIMEOUT_WATCH_READY); }); + it('should throttle invocations of command', function(done) { + var touch = 'touch ' + CHANGE_FILE; + var throttleTime = (2 * TIMEOUT_CHANGE_DETECTED) + 100; + + run('node ../index.js "dir/**/*.less" --debounce 0 --throttle ' + throttleTime + ' -c "' + touch + '"', { + pipe: DEBUG_TESTS, + cwd: './test', + callback: function(child) { + setTimeout(function killChild() { + // Kill child after test case + child.kill(); + killed = true; + }, TIMEOUT_KILL); + } + }); + + setTimeout(function afterWatchIsReady() { + fs.writeFileSync(resolve('dir/subdir/c.less'), 'content'); + setTimeout(function() { + assert(changeFileExists(), 'change file should exist after first change'); + fs.unlinkSync(resolve(CHANGE_FILE)); + fs.writeFileSync(resolve('dir/subdir/c.less'), 'more content'); + setTimeout(function() { + assert.equal(changeFileExists(), false, 'change file should not exist after second change'); + done(); + }, TIMEOUT_CHANGE_DETECTED); + }, TIMEOUT_CHANGE_DETECTED); + }, TIMEOUT_WATCH_READY); + }); + + it.skip('should debounce invocations of command', function(done) { + }); + + it.skip('should not run the command more than once concurrently', function(done) { + }); + + it.skip('should run the command concurrently when --concurrent is used', function(done) { + }); + it('should replace {path} and {event} in command', function(done) { var command = "echo '{event}:{path}' > " + CHANGE_FILE; From 78cf5cbc68806924dbba5068fecdd976534303d3 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Mon, 21 Mar 2016 13:31:41 -0700 Subject: [PATCH 05/11] implement debounce test and fix races --- test/test-all.js | 47 +++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 41 insertions(+), 6 deletions(-) diff --git a/test/test-all.js b/test/test-all.js index a3f86af..976d4cc 100644 --- a/test/test-all.js +++ b/test/test-all.js @@ -95,7 +95,8 @@ describe('chokidar-cli', function() { it('should throttle invocations of command', function(done) { var touch = 'touch ' + CHANGE_FILE; - var throttleTime = (2 * TIMEOUT_CHANGE_DETECTED) + 100; + var changedDetectedTime = 100; + var throttleTime = (2 * changedDetectedTime) + 100; run('node ../index.js "dir/**/*.less" --debounce 0 --throttle ' + throttleTime + ' -c "' + touch + '"', { pipe: DEBUG_TESTS, @@ -104,9 +105,11 @@ describe('chokidar-cli', function() { setTimeout(function killChild() { // Kill child after test case child.kill(); - killed = true; }, TIMEOUT_KILL); } + }) + .then(function childProcessExited(exitCode) { + done(); }); setTimeout(function afterWatchIsReady() { @@ -117,13 +120,45 @@ describe('chokidar-cli', function() { fs.writeFileSync(resolve('dir/subdir/c.less'), 'more content'); setTimeout(function() { assert.equal(changeFileExists(), false, 'change file should not exist after second change'); - done(); - }, TIMEOUT_CHANGE_DETECTED); - }, TIMEOUT_CHANGE_DETECTED); + }, changedDetectedTime); + }, changedDetectedTime); }, TIMEOUT_WATCH_READY); }); - it.skip('should debounce invocations of command', function(done) { + it('should debounce invocations of command', function(done) { + var touch = 'touch ' + CHANGE_FILE; + var changedDetectedTime = 100; + var debounceTime = (2 * changedDetectedTime) + 100; + var killTime = TIMEOUT_WATCH_READY + (2 * changedDetectedTime) + debounceTime + 1000; + + run('node ../index.js "dir/**/*.less" --debounce ' + debounceTime + ' -c "' + touch + '"', { + pipe: DEBUG_TESTS, + cwd: './test', + callback: function(child) { + setTimeout(function killChild() { + // Kill child after test case + child.kill(); + }, killTime); + } + }) + .then(function childProcessExited(exitCode) { + done(); + }); + + setTimeout(function afterWatchIsReady() { + fs.writeFileSync(resolve('dir/subdir/c.less'), 'content'); + setTimeout(function() { + assert.equal(changeFileExists(), false, 'change file should not exist earlier than debounce time (first)'); + fs.writeFileSync(resolve('dir/subdir/c.less'), 'more content'); + setTimeout(function() { + assert.equal(changeFileExists(), false, 'change file should not exist earlier than debounce time (second)'); + }, changedDetectedTime); + setTimeout(function() { + assert(changeFileExists(), 'change file should exist after debounce time'); + }, debounceTime + changedDetectedTime); + }, changedDetectedTime); + }, TIMEOUT_WATCH_READY); + }); it.skip('should not run the command more than once concurrently', function(done) { From a4c19b0bf33fe57e06f74c5740dcf6f2e109e81f Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Wed, 23 Mar 2016 13:16:04 -0700 Subject: [PATCH 06/11] fix bug binding path and exec to first change from watcher --- index.js | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/index.js b/index.js index 5a56db1..e19ea92 100755 --- a/index.js +++ b/index.js @@ -146,24 +146,19 @@ function getUserOpts(argv) { } function startWatching(opts) { + var child; var chokidarOpts = createChokidarOpts(opts); var watcher = chokidar.watch(opts.patterns, chokidarOpts); - var child; - var execFn; + var execFn = _.debounce(_.throttle(function(event, path) { + if (child) child.removeAllListeners(); + child = childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event)]); + child.once('error', function(error) { throw error; }); + child.once('exit', function() { child = undefined; }); + }, opts.throttle), opts.debounce); watcher.on('all', function(event, path) { var description = EVENT_DESCRIPTIONS[event] + ':'; - function executeCommand() { - if (!execFn) { - execFn = _.debounce(_.throttle(function() { - if (child) child.removeAllListeners(); - child = childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event)]); - child.once('error', function(error) { throw error; }); - child.once('exit', function() { child = undefined; }); - }, opts.throttle), opts.debounce); - } - execFn(); - } + var executeCommand = _.partial(execFn, event, path); if (opts.verbose) { console.error(description, path); From 22876934ec55179d771511c95c8b0185627a5e19 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Thu, 24 Mar 2016 23:58:38 -0700 Subject: [PATCH 07/11] exercised command spawn/kill in sample project, addressed issues --- index.js | 14 ++++++++------ package.json | 1 + test/test-all.js | 6 ------ 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/index.js b/index.js index e19ea92..fcbf45d 100755 --- a/index.js +++ b/index.js @@ -1,9 +1,9 @@ #!/usr/bin/env node -var childProcess = require('child_process'); var Promise = require('bluebird'); var _ = require('lodash'); var chokidar = require('chokidar'); +var spawn = require('npm-run-all/lib/spawn').default; var EVENT_DESCRIPTIONS = { add: 'File added', @@ -151,7 +151,13 @@ function startWatching(opts) { var watcher = chokidar.watch(opts.patterns, chokidarOpts); var execFn = _.debounce(_.throttle(function(event, path) { if (child) child.removeAllListeners(); - child = childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event)]); + child = spawn(SHELL_PATH, [ + EXECUTE_OPTION, + opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event) + ], + { + stdio: 'inherit' + }); child.once('error', function(error) { throw error; }); child.once('exit', function() { child = undefined; }); }, opts.throttle), opts.debounce); @@ -229,8 +235,4 @@ function _resolveIgnoreOpt(ignoreOpt) { }); } -function run(cmd) { - return childProcess.spawn(SHELL_PATH, [EXECUTE_OPTION, cmd]); -} - main(); diff --git a/package.json b/package.json index 7c4ebae..dbc5aeb 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "bluebird": "^2.9.24", "chokidar": "^1.0.1", "lodash": "^3.7.0", + "npm-run-all": "^1.6.0", "shell-quote": "^1.4.3", "yargs": "^3.7.2" }, diff --git a/test/test-all.js b/test/test-all.js index 976d4cc..6f19f15 100644 --- a/test/test-all.js +++ b/test/test-all.js @@ -161,12 +161,6 @@ describe('chokidar-cli', function() { }); - it.skip('should not run the command more than once concurrently', function(done) { - }); - - it.skip('should run the command concurrently when --concurrent is used', function(done) { - }); - it('should replace {path} and {event} in command', function(done) { var command = "echo '{event}:{path}' > " + CHANGE_FILE; From 3b63467e3c526bf53dc64e268d75cbef58e829c5 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Fri, 25 Mar 2016 00:06:09 -0700 Subject: [PATCH 08/11] lock to a specific version of npm-run-all --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index dbc5aeb..187d1ba 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "bluebird": "^2.9.24", "chokidar": "^1.0.1", "lodash": "^3.7.0", - "npm-run-all": "^1.6.0", + "npm-run-all": "1.6.0", "shell-quote": "^1.4.3", "yargs": "^3.7.2" }, From 42e84379fdcf1d102e289caa5dd4444a9be7c195 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Fri, 25 Mar 2016 10:52:02 -0700 Subject: [PATCH 09/11] 4 spaces --- index.js | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/index.js b/index.js index fcbf45d..6a6139c 100755 --- a/index.js +++ b/index.js @@ -152,11 +152,10 @@ function startWatching(opts) { var execFn = _.debounce(_.throttle(function(event, path) { if (child) child.removeAllListeners(); child = spawn(SHELL_PATH, [ - EXECUTE_OPTION, - opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event) - ], - { - stdio: 'inherit' + EXECUTE_OPTION, + opts.command.replace(/\{path\}/ig, path).replace(/\{event\}/ig, event) + ], { + stdio: 'inherit' }); child.once('error', function(error) { throw error; }); child.once('exit', function() { child = undefined; }); From 459a14991dc6040af2b0afc412cea0f748a33fc9 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Sat, 22 Dec 2018 15:19:34 -0800 Subject: [PATCH 10/11] fix an import by deleting the .default property based on: https://github.com/kimmobrunfeldt/chokidar-cli/pull/29#pullrequestreview-56586602 --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index 6a6139c..9218abe 100755 --- a/index.js +++ b/index.js @@ -3,7 +3,7 @@ var Promise = require('bluebird'); var _ = require('lodash'); var chokidar = require('chokidar'); -var spawn = require('npm-run-all/lib/spawn').default; +var spawn = require('npm-run-all/lib/spawn'); var EVENT_DESCRIPTIONS = { add: 'File added', From 7518c83bcc8f0f9580fc3ededc3ce759aa93fd56 Mon Sep 17 00:00:00 2001 From: Ankur Oberoi Date: Sat, 22 Dec 2018 17:13:32 -0800 Subject: [PATCH 11/11] Revert "fix an import by deleting the .default property" This reverts commit 459a14991dc6040af2b0afc412cea0f748a33fc9. --- index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/index.js b/index.js index 9218abe..6a6139c 100755 --- a/index.js +++ b/index.js @@ -3,7 +3,7 @@ var Promise = require('bluebird'); var _ = require('lodash'); var chokidar = require('chokidar'); -var spawn = require('npm-run-all/lib/spawn'); +var spawn = require('npm-run-all/lib/spawn').default; var EVENT_DESCRIPTIONS = { add: 'File added',