Run one process for command by default, add --concurrent option

[aoberoi]

addresses #20

adds a new boolean option --concurrent which, when true, behaves the same as the tool behaves today. when it is false (the default), it means the previous process launched for the command is killed before a new process is created again. i think this is a more sane default, because its more likely what users already expect. its more dangerous when the option is set to true because commands that recompile files might clobber the same file system state, or in my case, relaunching a server process will fail because the port will already be used.

i'm looking for feedback on this implementation, as i don't think its ready to merge yet. here is what i think is left before this can be merged:

• does the default value of false make sense? would that be considered "breaking"?
• okay to not use utils implementation of run()? i needed a different abstraction (as the existing run() implementation notes, a Promise is probably the wrong abstraction) which provided a reference to the child process, so that it could later be killed.
• need to implement tests for the new feature, but i don't have a means of getting the child process of the child process that the run() helper launches. might need to completely rethink the design of that helper (and its not even being reused so it can probably just live in the test/ directory).
• the code is naive and could probably be cleaned up. scopes/closure are probably leaned on too much. any suggestions?

i also added tests for the --throttle and --debounce options.

[kimmobrunfeldt]

I agree, this is a more sane default. However I'm not sure if killing the old process is the best solution, hmm. Yes the run in utils is a horrible and complicated abstraction. I've been hoping to find a good cross-platform solution for this which would support all shell goodies from the platform. E.g. which would use bash -c "dada" if you have it installed.

Other than those comments, looks good and thanks for taking time to make this.

[aoberoi]

@kimmobrunfeldt i think this PR is ready for review/merge.

i couldn't find a decent way to test the killing of the command child process (but i'll trade you for tests on debounce and throttle!).

instead, i exercised the functionality in a sample project. that lead to me finding several bugs and understanding that killing processes isn't as straight forward as it looks. my first attempt to fix was based on an attempt using https://github.com/paulpflug/better-spawn. this worked great everywhere if it weren't for a bug in OS X (details: nodejs/node#3596). next, i discovered a technique based on https://github.com/indexzero/ps-tree. it turns out that the most elegant implementation i could find of using this across platforms was inside the https://github.com/mysticatea/npm-run-all project. i decided that instead of rewriting that code, it made sense to just depend on it. i'm a tiny bit worried about depending on an internal implementation of that library, so i used an exact version in package.json so it doesn't risk breaking in the future.

let me know if there's anything else you'd like me to change before a merge!

[aoberoi]

@kimmobrunfeldt ping :)

[jacoscaz]

This is a lovely PR and exactly what I'm looking for. +1

[OliverJAsh]

@kimmobrunfeldt It's been over a year, is this PR good to merge? :-)

[corysimmons]

Added this here: https://github.com/corysimmons/chokidar-cli-infanticide

npm rm chokidar-cli
npm i chokidar-cli-infanticide
say hell yeah

Update Been using this all night. It's amazing being able to live reload Node servers...

[corysimmons]

.default doesn't work anymore. Just remove it and it works.

[zanona]

Oh dear, 2016, really?

[aoberoi]

it's been a while since i needed to use this package. thanks @corysimmons for finding that issue. i've now updating this branch, in case @kimmobrunfeldt comes back and decides to merge.

[aoberoi]

actually, reverting that previous update. this PR should still work as intended. i think @corysimmons might have run into an issue because he may have tried to update the npm-run-all dependency, whose API might have changed since the release i version-locked to. but i anticipated that, and that's why its version-locked 😄.

on a separate note, i've decided to take the "temporary" status off of my fork and commit to maintaining it. if you're interested, npm install @aoberoi/chokidar-cli is still the place to be.

[aoberoi]

A new project

Since this project seems to be moving along from this change, I wanted to let everyone know that I just published a new version from my fork.

Much gratitude for everyone involved in this project, and especially those who took the time to review this PR and give feedback. If anyone is interested in merging projects in the future, don't be a stranger!

[kimmobrunfeldt]

Hi! My maintenance efforts have been very low for this repo. To help with maintenance, I moved this repository under a new org, where I can add maintainers more easily. @aoberoi would you be interested to join the org? I'm spending now some efforts to update the most commonly used tools I've created in the past.