FSEvents watcher #97

amasad · 2017-05-30T02:12:45Z

There seems to be some kind of regression with the node watcher in MacOS serria:

This adds another watcher that consumers can use for MacOS.

amasad · 2017-05-30T02:13:34Z

amasad · 2017-05-30T03:10:11Z

Keep getting unknown from fsevents. Looks like we need to do a bit more ninja moves to handle this. Will pick this up later unless someone else wants to

ro-savage · 2017-05-30T13:16:04Z

src/fsevents_watcher.js

+    });
+  }
+
+  function emit(stat) {


All the this should be changed to self

yeah sorry fixed locally and didn't update the branch

ro-savage · 2017-05-30T13:51:51Z

A very quick test using the cra-desktop and fixing up the this / self bug it seemed to watch everything fine and without crashing.

There was an issue with creating files though. If I create a file using SublimeText, fsevents fired a modified event rather than a created event.

{ path: '/Users/Ro/www/create-react-app/test-watch/something.js',
  event: 'modified',
  type: 'file',
  changes: { inode: false, finder: true, access: false, xattrs: true },
  flags: 110848,
  id: 94649282 }

If I create a file with touch fsevent fires a unknown event

{ path: '/Users/Ro/www/create-react-app/test-watch/touch.js',
  event: 'unknown',
  type: 'file',
  changes: { inode: false, finder: false, access: false, xattrs: false },
  flags: 65792,
  id: 94649923 }

Tried both of these a few times, and got the same result each time.

thisconnect · 2017-05-30T15:09:02Z

@ro-savage the created/modified event seems to be known, see the comment in fsevent test

stefanpenner · 2017-05-30T15:16:39Z

src/fsevents_watcher.js

+
+FSEventsWatcher.prototype.emitEvent = function(type, file) {
+  var self = this;
+  console.log('here');


stefanpenner · 2017-05-30T15:17:28Z

src/fsevents_watcher.js

+  this.watcher.stop();
+  this.removeAllListeners();
+  if (typeof callback === 'function') {
+    setImmediate(callback.bind(null, null, true));


curious why setImmediate over process.nextTick ?

no reason this can be nextTick

stefanpenner · 2017-05-30T15:28:43Z

package.json

-  "homepage": "https://github.com/amasad/sane"
+  "homepage": "https://github.com/amasad/sane",
+  "optionalDependencies": {
+    "fsevents": "^1.1.1"


I'm nervous to introduce the node-gyp dependency here. Although it is an optional dependency the ergonomics around failed optionalDependencies is still quite poor for those with funky environments.

On a positive note, it looks like fsevents does seems to do its best to avoid pain here:

only run for OSX https://github.com/strongloop/fsevents/blob/master/binding.gyp#L6

only run in OSX https://github.com/strongloop/fsevents/blob/master/install.js#L1

uses node-pre-gyp to install a binary when possible.

One last concern is if the binaries themselves are not stored in the registry, so companies with private registries (for insulation from the outside world) might become unhappy. For this reason, if we proceed I would like to recommend this be at-least a major version bump.

Yeah agreed. I'm really reluctant to do it. But the problem is badd enough that the NodeWatcher is not usable on some machines (including mine).

@amasad if you gotta, mind doing a major version bump? or...

stefanpenner · 2017-05-30T15:29:24Z

src/fsevents_watcher.js

+    });
+  }
+
+  function emit(stat) {


amasad · 2017-05-30T17:44:45Z

@ro-savage yeah it seems like we need to handle these unknowns on our own. We need to keep a registry of files and deduce what changed. 😕

gaearon · 2017-05-30T19:17:24Z

Worth taking a look at how Chokidar uses fsevents? It's been working pretty well in webpack (even if it's not at FB scale).

ro-savage · 2017-05-31T07:59:40Z

Chokidar is doing exactly what @amasad mentioned.

It keeps an object that contains every item tracked and anytime fsevents fires a change/created/unknown event it checks it's registry of items and decided what actually happened.

Feels extreme, but it appears the only way to make sure its correct. So that's Option 1.

Option 2
You could also just fire a modified for all unknowns under the assumption that watchers mainly care if something changed rather that if its created or modified. And recommend people use Chokidar if they'd need to know for sure if something is created or modified.

(I actually have no idea of the implications of having an object with tens of thousands of properties in node, maybe it doesn't cause any issues with speed/memory)

Option 3
Change Jest and React-Native to use Chokidar and allow sane to keep doing what it does.

What's your thoughts @amasad ?

amasad · 2017-05-31T23:21:26Z

Option 1

Not extreme at all. We already do that for the node watcher. The reason Sane exists (and the reason for the name) is because native node watcher was garbage (it's still is I guess). See this.

Option 2

That's a non-starter -- the whole point of Sane is to paper over multiple watcher backends with the same API so that consumers can slide them in and out as they please.

Option 3

More than happy to deprecate if the major consumers think they can live without it /cc @cpojer, @stefanpenner, @gaearon et al.

My guess is that there is still value in the abstraction this provide so that you can switch between the different watchers depending on the platform and/or capabilities.

gaearon · 2017-05-31T23:47:26Z

@amasad Do you think you could find the time to finish up "option 1"?

amasad · 2017-05-31T23:49:06Z

@gaearon yeah, most likely do it this weekend.

gaearon · 2017-06-07T12:35:38Z

Friendly ping 😄

Do you think you could finish this, or maybe somebody else could help?
It would mean a lot to our users who are still struggling with those issues.

amasad · 2017-06-07T18:10:29Z

Yes made some progress. Ill cleanup, send an update on progress later today.

amasad · 2017-06-13T01:54:39Z

Ok this is passing tests and looks like its working well. I'll wait for comments then merge and release an alpha. Sorry for the delay @gaearon.

gaearon · 2017-06-13T23:09:28Z

No worries, thank you so much for the followup! Fingers crossed.

thisconnect · 2017-06-17T08:41:27Z

Is it correct that we have to modify jest on this line https://github.com/facebook/jest/blob/79eed2558402d660f914b47833658625399bbaff/packages/jest-haste-map/src/index.js#L569-L571
to test this sane version?

thisconnect · 2017-06-17T09:10:54Z

Ok success, tested facebook/create-react-app#2393 with repo https://github.com/thisconnect/cra-desktop

rm -rf node_modules/sane
rm -rf node_modules/jest-haste-map/node_modules/sane
npm i amasad/sane#fsevents
vi node_modules/jest-haste-map/build/index.js
# changes line 571
# from ": sane.NodeWatcher;"
# to "sane.FSEventsWatcher;"
npm t 
# WORKS with no error!

no error
changing a file in node_modules seems to re-trigger the tests
the tests in src work

testing the error one more time

rm -rf node_modules && npm i && npm t
# old error works too :)

ro-savage · 2017-06-17T11:17:18Z

Sorry been on holidays.

Tested it with a Create React App where I just installed a bunch of large npm packages. Came out at 1200+ packages and approximately 50,000 files.

Died on old sane. Worked fine on this branch.

I'd say go ahead and release.

MylesBorins · 2017-06-17T13:48:02Z

hey all... is there something that core can do to hell with this regression? we are getting some new test infra for osx soon, which should hell find regressions.

gaearon · 2017-06-22T16:58:37Z

Seems to work in my testing! Is there anything else holding this back?

thisconnect · 2017-06-22T18:15:11Z

@ro-savage and @gaearon did you patch jest-hastle-maps as in #97 (comment)

or was sane a drop in replacement?

gaearon · 2017-06-22T18:28:34Z

I did patch that file, yes.

amasad · 2017-06-23T20:26:12Z

@gaearon I'm OOO. But I'll find time for the release. Sorry bout the hold up.

@MylesBorins thanks for the offer. I think the most important thing is to test against really large directories. This is vague in my memory but my understanding is that I you look at watchman and the npm module fsevents does a better job at reusing watches (and even multiplexing I the case of watchman) where is native node watcher is resource hungry.

amasad · 2017-06-25T01:02:31Z

Merged. And sent a PR to jest. jestjs/jest#3902

This is to fix regressions with the NodeWatcher on macOS. More info here: amasad/sane#97

FSEvents watcher

a7c287e

amasad requested a review from stefanpenner May 30, 2017 02:13

remove only

9a789f9

amasad changed the title ~~FSEvents watcher~~ [wip] FSEvents watcher May 30, 2017

amasad mentioned this pull request May 30, 2017

Fix Jest crashing on projects with large node_modules folders facebook/create-react-app#2395

Closed

gaearon mentioned this pull request May 30, 2017

react-scripts test is watching files in node_modules facebook/create-react-app#2393

Closed

ro-savage reviewed May 30, 2017

View reviewed changes

stefanpenner requested changes May 30, 2017

View reviewed changes

fixes

edf73bc

gaearon mentioned this pull request Jun 10, 2017

Watch mode stopped working on macOS Sierra jestjs/jest#1767

Closed

amasad added 3 commits June 12, 2017 18:42

Track files and deduce change

5075135

move tests

fa54192

nextTick

248a8c1

amasad changed the title ~~[wip] FSEvents watcher~~ FSEvents watcher Jun 13, 2017

amasad added 2 commits June 12, 2017 18:56

test

006e88c

no need for prepublish now that we have CI

3875a72

thisconnect mentioned this pull request Jun 17, 2017

Error: Error watching file for changes: EMFILE jestjs/jest#3436

Closed

gaearon mentioned this pull request Jun 22, 2017

npm test hangs for newly generated app facebook/create-react-app#960

Closed

amasad merged commit 803407c into master Jun 25, 2017

amasad deleted the fsevents branch June 25, 2017 00:28

amasad mentioned this pull request Jun 25, 2017

Upgrade sane and use FSEventsWatcher on Darwin jestjs/jest#3902

Closed

SimenB pushed a commit to SimenB/jest that referenced this pull request Jun 26, 2017

Upgrade sane and use FSEventsWatcher on Darwin

bff25d2

This is to fix regressions with the NodeWatcher on macOS. More info here: amasad/sane#97

SimenB mentioned this pull request Jun 26, 2017

Upgrade sane and use FSEventsWatcher on Darwin jestjs/jest#3918

Merged

cpojer pushed a commit to jestjs/jest that referenced this pull request Jun 27, 2017

Upgrade sane and use FSEventsWatcher on Darwin (#3918)

acc783a

This is to fix regressions with the NodeWatcher on macOS. More info here: amasad/sane#97

tushardhole pushed a commit to tushardhole/jest that referenced this pull request Aug 21, 2017

Upgrade sane and use FSEventsWatcher on Darwin (jestjs#3918)

9c97c86

This is to fix regressions with the NodeWatcher on macOS. More info here: amasad/sane#97

stefanpenner mentioned this pull request Sep 6, 2018

Revert "[PERF] Speed up package info cache" ember-cli/ember-cli#8019

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FSEvents watcher #97

FSEvents watcher #97

amasad commented May 30, 2017 •

edited

amasad commented May 30, 2017

amasad commented May 30, 2017

ro-savage May 30, 2017

stefanpenner May 30, 2017

amasad May 30, 2017

ro-savage commented May 30, 2017

thisconnect commented May 30, 2017 •

edited

stefanpenner May 30, 2017

stefanpenner May 30, 2017

amasad May 30, 2017

stefanpenner May 30, 2017

amasad May 30, 2017

stefanpenner May 30, 2017

amasad May 30, 2017

stefanpenner May 30, 2017

amasad commented May 30, 2017

gaearon commented May 30, 2017 •

edited

ro-savage commented May 31, 2017 •

edited

amasad commented May 31, 2017

gaearon commented May 31, 2017

amasad commented May 31, 2017

gaearon commented Jun 7, 2017

amasad commented Jun 7, 2017

amasad commented Jun 13, 2017

gaearon commented Jun 13, 2017

thisconnect commented Jun 17, 2017 •

edited

thisconnect commented Jun 17, 2017 •

edited

ro-savage commented Jun 17, 2017 •

edited

MylesBorins commented Jun 17, 2017

gaearon commented Jun 22, 2017

thisconnect commented Jun 22, 2017

gaearon commented Jun 22, 2017

amasad commented Jun 23, 2017

amasad commented Jun 25, 2017

FSEvents watcher #97

FSEvents watcher #97

Conversation

amasad commented May 30, 2017 • edited

amasad commented May 30, 2017

amasad commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro-savage commented May 30, 2017

thisconnect commented May 30, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amasad commented May 30, 2017

gaearon commented May 30, 2017 • edited

ro-savage commented May 31, 2017 • edited

amasad commented May 31, 2017

gaearon commented May 31, 2017

amasad commented May 31, 2017

gaearon commented Jun 7, 2017

amasad commented Jun 7, 2017

amasad commented Jun 13, 2017

gaearon commented Jun 13, 2017

thisconnect commented Jun 17, 2017 • edited

thisconnect commented Jun 17, 2017 • edited

ro-savage commented Jun 17, 2017 • edited

MylesBorins commented Jun 17, 2017

gaearon commented Jun 22, 2017

thisconnect commented Jun 22, 2017

gaearon commented Jun 22, 2017

amasad commented Jun 23, 2017

amasad commented Jun 25, 2017

amasad commented May 30, 2017 •

edited

thisconnect commented May 30, 2017 •

edited

gaearon commented May 30, 2017 •

edited

ro-savage commented May 31, 2017 •

edited

thisconnect commented Jun 17, 2017 •

edited

thisconnect commented Jun 17, 2017 •

edited

ro-savage commented Jun 17, 2017 •

edited