Switch to chokidar #13

es128 · 2015-01-22T12:29:13Z

io.js compatibility (update/correction: gaze 0.5.1 is compatible with io.js also, but 0.6.* isn't)
not as buggy

This is aimed at establishing parity with prior behavior using gaze, except for swapping out old for stats as the extra argument coming from the watcher's events.

Switching to chokidar's regular event names (sooner or later) would be fine too if it wouldn't be too disruptive for downstream users of this module to adjust.

Please evaluate and let me know if there's any issue or concern. If not, I can release chokidar 1.0.0 before you push this out.

paulmillr · 2015-01-22T19:51:18Z

@contra in fact, Gaze has 30+ issues while we have two.

TrySound · 2015-01-22T20:02:06Z

Woohoo!

heikki · 2015-01-22T20:08:07Z

Some (10) chokidar tests fail on windows. Also a little tweak was necessary to run tests:
"test": "istanbul test node_modules/mocha/bin/_mocha"

es128 · 2015-01-22T20:48:07Z

@heikki Thanks for pointing that out, forgot to run the windows tests recently.

Fixed in paulmillr/chokidar@ecace6c. It was really just the one relative paths test failing and throwing off the fixture cleanup, messing up the rest of the tests.

heikki · 2015-01-22T21:00:36Z

@es128 Tests pass now 🎉

For some reason npm test fix paulmillr/chokidar@48387fb doesn't work for me.
This path works: node_modules/mocha/bin/_mocha.

es128 · 2015-01-22T21:02:09Z

grr... it's really istanbul's fault

w/e I'll change it

gulpjs/glob-watcher#13 (comment) 70

heikki · 2015-01-22T21:21:11Z

Btw, there's appveyor if you want to try automating windows tests. For example liftoff and gulp-cli use it.

yocontra · 2015-01-22T22:23:51Z

index.js

-      out.emit('change', outEvt);
-      if(cb) cb(outEvt);
-    });
+  if(opts.ignoreInitial === undefined) opts.ignoreInitial = true;


whats this new option?

without it, add events are emitted upon instantiation

if (opts.ignoreInitial == null)

yocontra · 2015-01-22T22:29:29Z

Lots of minor style things, some probably not your fault (this repo is old, old style guidelines). I would check out the vinyl-fs repo for what our current style looks like

es128 · 2015-01-22T22:32:49Z

Yeah thanks for the review. I was matching existing style, so perhaps all that should be done as a separate PR since it impacts more than just my changes?

If I do it all here, it will become hard to tell which part of the diff is relevant to switching to chokidar.

yocontra · 2015-01-22T22:35:50Z

Still need to see some benchmarks before switching

@phated @sindresorhus any thoughts?

sindresorhus · 2015-01-22T22:59:42Z

Probably yes, after seeing benchmarks.

xzyfer · 2015-01-22T23:18:16Z

FWIW the navelgazer has some benchmarks.

Library versions aren't mentioned. YMMV.

paulmillr · 2015-01-23T01:16:03Z

I've updated the chokidar to the 1.0.0-rc2 in this benchmark locally and tested it. @xzyfer

It's a synthetic piece of shit. We do setTimeout ... 50 throttling for change events to make everything work perfectly. It says chokidar takes 51ms. Make your judgement. How about benchmarking stability?

yocontra · 2015-01-23T01:39:06Z

@paulmillr Why do you need to throttle events at 50ms to make things work? Seems like a bandaid for a race condition somewhere

es128 · 2015-01-23T01:40:57Z

The throttle doesn't delay the change event, it suppresses duplicates. There is a delay on unlinks so that if the file comes back quickly the whole transaction gets emitted as a change.

The navelgazer benchmark does not show results for current versions of gaze, so even if accurate it isn't a fair comparison. I don't expect chokidar to react to anything in the sub-ms range. The priority has been stability and consistency, and perf is likely to vary widely between platforms. All the underlying watch methods are liars, so there are many cases where the true state of things needs to be double-checked by getting a stat or readdir before emitting.

I'll see what I can do about adapting gaze's benchmark scripts so we can get a reasonable idea of how they actually compare.

es128 · 2015-01-23T01:43:31Z

Yes, there are race condition-y things that happen at the file system level that have to be corrected for. Especially when it comes to all the text editors that use atomic write methods.

yocontra · 2015-01-23T01:52:07Z

@es128 Yeah, I think creating an accurate benchmark would be handy and then we can run it on all platforms to see if there are any irregularities that have to be fixed

heikki · 2015-01-23T02:10:51Z

Results change big time when testing separately:

∴ navelgazer git:(master) node benchmarks/change.js 

Benchmarking single change on darwin-x64-v8-3.14...
-------------------------------------------------------
chokidar@1.0.0-rc2:     1.82ms
-------------------------------------------------------

∴ navelgazer git:(master) node benchmarks/change.js 

Benchmarking single change on darwin-x64-v8-3.14...
-------------------------------------------------------
navelgazer@1.0.0:   1.55ms
-------------------------------------------------------

∴ navelgazer git:(master) node benchmarks/change.js 

Benchmarking single change on darwin-x64-v8-3.14...
-------------------------------------------------------
fs.watch@v0.10.35:  2.25ms
-------------------------------------------------------

∴ navelgazer git:(master) node benchmarks/change.js 

Benchmarking single change on darwin-x64-v8-3.14...
-------------------------------------------------------
watch@0.11.0:       4,005.98ms
-------------------------------------------------------

∴ navelgazer git:(master) node benchmarks/change.js 

Benchmarking single change on darwin-x64-v8-3.14...
-------------------------------------------------------
pathwatcher@2.1.3:  2.49ms
-------------------------------------------------------

es128 · 2015-01-23T02:14:35Z

Well I'll say preemptively that OS X, with the fsevents API, is likely to turn out pretty solid, but linux and windows are backed by fs.watch by default and may not fare as well - anecdotally I'll assert that it's good enough for anyone's real-life use-cases though.

I don't expect there is any potential for dramatic gains without reintroducing problems or adopting new underlying watch methods. Which is what brings us back around to why @shama has taken the approach he has with navelgazer, which sounds like will be pretty awesome if he can stabilize its problems.

In the meantime, weighing performance and stability, I believe chokidar is the best option. I think I've more or less proven the stability side already, will work on putting some legit numbers to the perf side.

coveralls · 2015-04-07T20:20:57Z

Coverage decreased (-6.25%) to 93.75% when pulling d965658 on es128:chokidar into 103840f on wearefractal:master.

coveralls · 2015-04-07T20:31:24Z

Coverage decreased (-6.25%) to 93.75% when pulling fca10f2 on es128:chokidar into 103840f on wearefractal:master.

coveralls · 2015-04-07T20:31:30Z

Coverage decreased (-6.25%) to 93.75% when pulling fca10f2 on es128:chokidar into 103840f on wearefractal:master.

es128 · 2015-04-07T20:35:29Z

The coverage issue has to do with the backward-compatible event mapping (meaning events aren't just passed through like before).

Coverage could be increased by either

adding tests that add a new file, delete a watched file, and add or remove a dir; or
removing the mapping and pass through event details like before, but that would be a breaking change because then chokidar's event names are different from gaze's

phated · 2015-04-08T00:16:39Z

This is going to end up in gulp4 so we can make breaking changes.

es128 · 2015-04-08T00:19:25Z

So are you saying you'd like me to adopt chokidar's event names and follow option 2?

phated · 2015-04-08T00:24:42Z

I'd say yes because it keeps the code small, but you should wait for @contra to chime in.

yocontra · 2015-04-08T00:31:13Z

Traveling right now, will look at this when I get a minute

coveralls · 2015-04-08T12:54:47Z

Coverage decreased (-6.12%) to 93.88% when pulling a107487 on es128:chokidar into 103840f on wearefractal:master.

es128 · 2015-04-08T12:58:04Z

Sorry about the coveralls spam. Realized I hadn't done the return statement in the end method to keep it consistent with before: https://github.com/wearefractal/glob-watcher/pull/13/files#diff-168726dbe96b3ce427e7fedce31bb0bcR77

However, an argument could be made that all the methods should actually be returning out to make glob-watcher's API properly chainable.

yocontra · 2015-08-18T00:42:39Z

index.js

-      out.emit('change', outEvt);
-      if(cb) cb();
-    });
+function mapEvents(evt) {


would be much better as an object

eventMap = { add: 'added', ... }

es128@ceb4af0

yocontra · 2015-08-18T00:44:27Z

left some comments on the code. code needs cleanup and some comments so it is clear, new options need documentation. good to switch to chokidar after that is done

phated · 2015-08-18T02:44:27Z

I think we should remove glob-watcher, as we don't need another abstraction on chokidar (it watches globs now)

phated · 2015-08-18T02:45:04Z

vinyl-fs could just include chokidar as watch (or remove it all together... we should open an issue about that on vinyl-fs to discuss)

paulmillr · 2015-08-18T08:36:48Z

👍

es128 · 2015-08-18T15:49:18Z

Updated the PR per @contra's feedback in case you do want to continue to use glob-watcher as-is without any breaking API changes.

If the decision is to make more structural changes of the sort @phated is getting at, I could get behind that notion too.

One particular inconsistency with glob-watcher's current API is that method calls return the underlying watcher instance instead of glob-watcher's own EventEmitter instance (under the identifier out in the code), which could lead to some odd unexpected behavior if anyone were to try to chain methods.

yocontra · 2015-08-18T19:49:59Z

@es128 I'm down to break some stuff, this will land in gulp 4 anyways which already has breaks. Feel free to change whatever you want, and make a list of all of the changes + flag the breaking ones and I'll review it 🌴

es128 · 2015-08-18T20:00:18Z

Cool, and I'm happy to help this along, but would like to better understand your thoughts on what an ideal greenfield implementation of chokidar for gulp.watch would look like.

You might be referring to small breaking changes like my note about the return values from the methods here. Or we could switch to using chokidar's event names directly without mapping. Or it might make sense to set glob-watcher aside and just implement chokidar directly in gulp or vinyl-fs for use with gulp.watch.

I don't have enough expertise with gulp to have strong opinions on many of these matters. Please let me know how you think we should proceed.

yocontra · 2015-08-18T20:06:05Z

@es128 Shelving glob-watcher and just doing it in vinyl-fs is fine for me. Also, no event mapping and returning for chaining seems fine too.

phated · 2015-08-18T20:18:38Z

Ref gulpjs/vinyl-fs#86

phated · 2015-10-20T03:14:11Z

Closing since we are using chokidar directly in gulp 4

es128 mentioned this pull request Jan 22, 2015

File watcher has problems gulpjs/gulp#651

Closed

paulmillr mentioned this pull request Jan 22, 2015

Migrate to a better file watcher sass/node-sass#636

Open

es128 added a commit to paulmillr/chokidar that referenced this pull request Jan 22, 2015

npm test for windows again

dc58a3c

gulpjs/glob-watcher#13 (comment) 70

yocontra reviewed Jan 22, 2015
View reviewed changes

es128 force-pushed the chokidar branch from d965658 to fca10f2 Compare April 7, 2015 20:28

es128 mentioned this pull request Apr 7, 2015

Gulp wrapper paulmillr/chokidar#200

Closed

Switch to chokidar

a107487

es128 force-pushed the chokidar branch from fca10f2 to a107487 Compare April 8, 2015 12:52

es128 mentioned this pull request Jul 28, 2015

Watch in a child process paulmillr/chokidar#318

Closed

yocontra reviewed Aug 18, 2015
View reviewed changes

es128 added 2 commits August 18, 2015 11:26

Use object for event mapping

ceb4af0

Add notes about chokidar options

26e6438

phated closed this Oct 20, 2015

Switch to chokidar #13

Switch to chokidar #13

Conversation

es128 commented Jan 22, 2015

paulmillr commented Jan 22, 2015

TrySound commented Jan 22, 2015

heikki commented Jan 22, 2015

es128 commented Jan 22, 2015

heikki commented Jan 22, 2015

es128 commented Jan 22, 2015

heikki commented Jan 22, 2015

yocontra Jan 22, 2015

Choose a reason for hiding this comment

es128 Jan 22, 2015

Choose a reason for hiding this comment

yocontra Jan 22, 2015

Choose a reason for hiding this comment

yocontra commented Jan 22, 2015

es128 commented Jan 22, 2015

yocontra commented Jan 22, 2015

sindresorhus commented Jan 22, 2015

xzyfer commented Jan 22, 2015

paulmillr commented Jan 23, 2015

yocontra commented Jan 23, 2015

es128 commented Jan 23, 2015

es128 commented Jan 23, 2015

yocontra commented Jan 23, 2015

heikki commented Jan 23, 2015

es128 commented Jan 23, 2015

coveralls commented Apr 7, 2015

coveralls commented Apr 7, 2015

coveralls commented Apr 7, 2015

es128 commented Apr 7, 2015

phated commented Apr 8, 2015

es128 commented Apr 8, 2015

phated commented Apr 8, 2015

yocontra commented Apr 8, 2015

coveralls commented Apr 8, 2015

es128 commented Apr 8, 2015

yocontra Aug 18, 2015

Choose a reason for hiding this comment

es128 Aug 18, 2015

Choose a reason for hiding this comment

yocontra commented Aug 18, 2015

phated commented Aug 18, 2015

phated commented Aug 18, 2015

paulmillr commented Aug 18, 2015

es128 commented Aug 18, 2015

yocontra commented Aug 18, 2015

es128 commented Aug 18, 2015

yocontra commented Aug 18, 2015

phated commented Aug 18, 2015

phated commented Oct 20, 2015