Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Packager] Switch file watcher to Chokidar #628

Closed
paulmillr opened this issue Apr 2, 2015 · 37 comments
Closed

[Packager] Switch file watcher to Chokidar #628

paulmillr opened this issue Apr 2, 2015 · 37 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@paulmillr
Copy link

Hey. You are currently using sane for file watching. How about using chokidar?

  1. Community:
    • Sane was downloaded 98K times last month.
    • Chokidar was downloaded 58K times just yesterday. It is also used by socketstream and stuff like Gulp is switching to it right now.
  2. Bugs:
    • Chokidar is obviously more tested in real-world applications - we have a solid track of 24m+ stable releases. We are not aware of any "node watch bugs" you mention on the web site — with correct implementation. Chokidar also has huge test coverage.
  3. Speed:
    • With Chokidar, you may use our fastest available polling for OSX with FSEvents. Sane also uses FSevents, but through child_process. This is NOT fast and also error-prone. Chokidar uses FSevents with native calls.

I know that Sane is made by facebook dude, but how about trying more stable & robust solution?

@vkurchatkin
Copy link
Contributor

#5 (comment)

@paulmillr
Copy link
Author

@vkurchatkin

Additionally chokidar has so many problems but it's suffice to say it doesn't work on large codebases like ours.

They fixed a lot of issues in 1.0. I used to find it unreliable for months, but I suggest you give it another try unless you're absolutely sure.

He has the point. If chokidar has serious problems, why doesn't anyone report them? Are you absolutely sure that sane (which uses child_process) would handle large codebases without issues?

@amasad
Copy link
Contributor

amasad commented Apr 2, 2015

Hey. You are currently using sane for file watching. How about using chokidar?

We're primarily interested using watchman. Sane is just use for a fallback. Watchman is battle tested on facebook infrastructure, developer laptops and many open source users.

I'm still open to exploring that option but I believe that watchman is a much better technology.

Chokidar was downloaded 58K times just yesterday. It is also used by socketstream and stuff like Gulp is switching to it right now.

That's great. But by the same standards watchman is a much popular open source project than chokidar, it has 2.5k github stars while chokidar has 700 :)

Chokidar is obviously more tested in real-world applications - we have a solid track of 24m+ stable releases.

Again, watchman is also battle tested 36 releases :)

We are not aware of any "node watch bugs" you mention on the web site — with correct implementation. Chokidar also has huge test coverage.

Yes, because you use a native fsevents module.

@amasad
Copy link
Contributor

amasad commented Apr 2, 2015

He has the point. If we have serious problems, why doesn't anyone report them? Are you absolutely sure that sane (which uses child_process) would handle large codebases without issues?

I did report them in the past. It was incredibly slow to use on our codebase, I needed a ready event which I sent a pull request for that wasn't accepted. Even with the ready event it was no match to watchman's instant daemon model.

Also "child_process" is dishonest. We only exec to show an error message that watchman isn't installed.

@paulmillr
Copy link
Author

Would be great to go full NPM without homebrew though — opened the issue facebook/watchman#91

Also "child_process" is dishonest.

Right. Sorry for that, the import confused me.

@amasad what about that?

  // We need to ask the client binary where to find it.
  // This will cause the service to start for us if it isn't
  // already running.
  var watchmanCommand = this.watchmanBinaryPath + ' get-sockname';
  childProcess.exec(watchmanCommand,

@es128
Copy link

es128 commented Apr 2, 2015

@amasad Do you perceive that there would be value in adding a watchman option to chokidar for users willing to do the separate non-npm install?

If we were to move in that direction, is there potential for consolidating efforts toward a single library, or are there any other concerns/shortcomings with the recent versions of chokidar that would prevent you from considering this?

@amasad
Copy link
Contributor

amasad commented Apr 2, 2015

Would be great to go full NPM without homebrew though — opened the issue facebook/watchman#91

Maybe @wez can think of a way this could work. But the way watchman works -- and what makes it fast and reliable -- is that it's a daemon.

@amasad is that the exec check?

So this shells out to get the socket name and then just uses unix sockets to connect to watchman.

@amasad Do you perceive that there would be value in adding a watchman option to chokidar for users willing to do the separate non-npm install?

If we were to move in that direction, is there potential for consolidating efforts toward a single library, or are there any other concerns/shortcomings with the recent versions of chokidar that would prevent you from considering this?

Yes, this sounds great. If chokidar has all the backends/fallbacks that sane has then there is no reason for sane to exist.

@es128
Copy link

es128 commented Apr 2, 2015

Yes, this sounds great. If chokidar has all the backends/fallbacks that sane has then there is no reason for sane to exist.

Well let me just finish pushing 1.0.0 out the door (should be next week), but after that I'd be happy to collaborate on giving this idea a try.

@amasad
Copy link
Contributor

amasad commented Apr 2, 2015

Here is the watching backends that would be great to support in this consolidated effort:

  1. watchman -- used for high performance servers and large codebases
  2. fsevents (what chokidar has) -- used for the common case where the user is on OS X and can install native module
  3. node fs.watch (still better than polling) -- used for common linux and windows users
  4. polling (currently done via the watch module on sane) -- used for environments where no native fs events exists, e.g. vagrant

Thoughts?

@es128
Copy link

es128 commented Apr 2, 2015

By the way, one of the remaining pain points remaining on using chokidar with extremely large file trees isn't the actual watching (unless using polling) - it's the need to scan the entire tree at instantiation as a basis for normalizing events. Currently use readdirp for this, have been thinking about submitting a patch there to create a "fast mode" option that avoids getting a stat for each file.

@paulmillr
Copy link
Author

@amasad sounds great

@es128
Copy link

es128 commented Apr 2, 2015

Well yeah - chokidar already has great support for 2-4 on your list. The effort would be to add watchman, and preferably detect its presence to have it kick in by default when available - otherwise fall back to existing defaults.

@es128
Copy link

es128 commented Apr 2, 2015

Also, regarding polling, see paulmillr/chokidar#242. Been mulling over how to solve this as well. Want to cover as many cases as possible where we can get it to "just work" without any environment-specific settings needed.

@wez
Copy link

wez commented Apr 2, 2015

I'd be happy to help you figure out how to consume watchman from chokidar. For large trees and multiple tools wanting to consume file notifications, using watchman as a consolidation point will reduce overall resource utilization and avoid node-process startup time overheads (particularly if you restart your node process).

@amasad
Copy link
Contributor

amasad commented Apr 2, 2015

@es128 cool! It seems like chokidar has really improved in the year that I hadn't looked. Let's make this happen.

cc @stefanpenner any concerns?

@wez
Copy link

wez commented Apr 2, 2015

Strongly recommend that you use the new (just landed) watch-project command for your integration to get the best consolidation:
https://facebook.github.io/watchman/docs/cmd/watch-project.html
don't hesitate to ask me if you have questions; you can find me on the #watchman IRC channel on freenode if you have more realtime questions (although I'm not always at my desk)

@es128
Copy link

es128 commented Apr 2, 2015

has really improved in the year that I hadn't looked

haha yeah well I've been putting a lot of effort into it since around October

@es128
Copy link

es128 commented Apr 2, 2015

@wez Cool - thanks for the tips

@stefanpenner
Copy link

cc @stefanpenner any concerns?

Without digging in further, the only superficial concern is the fsevent c-extension chokidar has is a pretty hefty dependency. Especially for tooling authors, who have consumers that may not have the required toolchains installed or experience to debug available.

Maybe node-gyp / chokidar have improved dramatically since I last used them, and my concerns are now pointless.

I'll try to find some time in the next few days to play, and come up with some concrete feedback.

@es128
Copy link

es128 commented Apr 2, 2015

@stefanpenner It's worth noting that fsevents is a optional dependency for chokidar. It continues to function adequately without it. And lots of people are using this today, very few have problems or concerns.

There's also an effort underway in the fsevents project to speed up installs using node-pre-gyp to distribute pre-compiled binaries.

@stefanpenner
Copy link

It continues to function adequately without it.

If it fails to build, it does create an extremely noisy/confusing experience.

  • what is all this output
  • what did i do wrong
  • etc etc.

It is a pretty disheartening experience, for an optional piece of infrastructure to draw so much attention to itself, when it fails.

@es128
Copy link

es128 commented Apr 2, 2015

Yes I agree. And I deal with it when people come asking for help. At this point the answers are easily google-able.

Still, wish npm handled it better.

@stefanpenner
Copy link

Yes I agree. And I deal with it when people come asking for help. At this point the answers are easily google-able.

this is sub-optimal, it reflects poorly on us (tooling maintainers) and increases pain and noise in our issue trackers. It is our responsibility to provide frictionless experiences, so developers can just do their actual work.

Theoretical ideal resolutions:

  • get improved fsevent handling upstreamed to libuv
  • work with NPM to handle this better (as @es128 mentioned, although we should likely elaborate )
  • bundle binaries and use sockets to communicate...

although these solutions will likely not satisfy short-term use-cases, it would be great to try and get the ball rolling so someday we can just have nice things.

@amasad
Copy link
Contributor

amasad commented Apr 2, 2015

get improved fsevent handling upstreamed to iojs/node

Didn't anyone check to see if things got better in node v0.12 or iojs?

@es128
Copy link

es128 commented Apr 2, 2015

Yes. And it didn't. Or, maybe better. But still far from adequate.

@stefanpenner
Copy link

Yes. And it didn't. Or, maybe better. But still far from adequate.

is there a relevant issue open on libuv?

@es128
Copy link

es128 commented Apr 2, 2015

just have nice things

Progress is being made all the time, but there will always be issues and more to improve/solve.

@es128
Copy link

es128 commented Apr 2, 2015

is there a relevant issue open on libuv?

There's plenty of discussions I've seen in the node and libuv issue trackers that tend to culminate in "we're exposing what the OS provides, nothing more we can do". Here's a more recent one in io.js that is somewhat relevant: nodejs/node#375

And there's actually a reasonable point they're making there for many circumstances. For instance, editors often cause a lot of churn when they save files, and I'm not sure it's core's job to normalize that kind of thing. But to provide a smooth experience in tooling, the normalization is necessary. So there will likely remain an ongoing place for higher-level userspace modules that do this sort of work.

@stefanpenner
Copy link

Ya the editor induced churn is painful. Maybe watchman itself we can push through the ecosystem to help facilitate better experiences.

I suspect it fairly unlikely our os vendors will be likely to improve in the short term, but maybe it's worth dreaming?

@amasad
Copy link
Contributor

amasad commented Apr 13, 2015

This long running chokidar issue was brought to my attention. I'm going to leave a mention of it here for posterity paulmillr/chokidar#219

@stefanpenner
Copy link

In my experience, the watchman process is the most stable form of watching I have yet to encounter.

I re-visited the c extension problem and the issue persists. New users are extremely confused, often believing the entire attempt failed or at best assuming a quality issue in the tool they use.
In hopes of dealing with this issue at the source, I have opened the following issue with NPM: npm/npm#7892 I believe this is primarily a UX issue, hopefully it can be sorted out.

One downside though, is that if the c-extension fails, the resulting watching experience may be as broken as the c-extension failing implied :(

2015 and we have to resort to hacks to get meaningful information change events form the FS. What a sad state of affairs.

@es128
Copy link

es128 commented Apr 13, 2015

@stefanpenner the npm UX issue is different from the one @amasad is referring to.

There is an issue in fsevents when it's bombarded with simultaneous events in some specific circumstances that sometimes causes processes to crash with a segfault - notably doing rm -rf node_modules && npm i with a big enough dep tree. @bnoordhuis has been putting some time in toward trying to resolve it, although that effort is still in progress. Unfortunately I don't have the C chops to do something about it myself, and I hope we're eventually able to sort it out.

Nonetheless, as has been discussed in this thread, looking forward to adding a watchman interface to chokidar which would provide another option for mediating this and other problems that still exist with the lower-level watch methods currently being relied upon.

if the c-extension fails, the resulting watching experience may be as broken as the c-extension failing implied

I disagree with this. In this case it falls back to the same code that works very well on Linux and Windows. It's not as good as it would be with fsevents, but it's not flat-out broken. The test suite is run using the fs.watch and fs.watchFile modes on all three platforms.

@stefanpenner
Copy link

Yes. I am raising an additional concern.

@es128
Copy link

es128 commented Apr 13, 2015

For the most part, this impacts Mac users who haven't installed XCode (or the command-line tools). Is that who you're focusing on by raising the concern? Or is there another persona I'm unaware of that struggles with this?

At least as much savvy is required to go out and install watchman as to resolve the XCode thing. Which is why facebook/watchman#91 would be nice.

Also, fsevents/fsevents#59 is in progress which may largely alleviate this problem as it relates to chokidar/fsevents.

@stefanpenner
Copy link

There is another cohort, one with Xcode installed but with a broken compiler tool chain. This is sadly common with those with enough context with the tech to be dangerous but insufficient context or time to resolve. This can easily occur when upgrading An OS, or botching something as simple as env $PATH.

Although it would be nice to re-delegate this issue back to the user in question. Our goal should be to reduce complexity/ friction/ noise not to compound it unless absolutely needed.

I realize I am likely coming off as a pain, but my motivations are really just to pave the pain points I observe from my users.

@es128
Copy link

es128 commented Apr 13, 2015

Our goal should be to reduce complexity/ friction/ noise not to compound it.

Absolutely, to whatever extent is possible

@brentvatne brentvatne changed the title Switch file watcher to Chokidar [Packager] Switch file watcher to Chokidar May 31, 2015
@amasad
Copy link
Contributor

amasad commented Jun 2, 2015

Going to close because it's not really actionable (we can continue to discuss). Let's revisit if once we have a plan to address the issues above.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests

7 participants