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

Consider moving from chokidar to sane. #36

Closed
wtgtybhertgeghgtwtg opened this issue Dec 8, 2016 · 20 comments
Closed

Consider moving from chokidar to sane. #36

wtgtybhertgeghgtwtg opened this issue Dec 8, 2016 · 20 comments

Comments

@wtgtybhertgeghgtwtg
Copy link
Contributor

sane is the filesystem watcher used by jest-cli. It supports Linux, OS X, and Windows, and can use watchman if available. More notably, its dependency tree is significantly smaller than the dependency tree for chokidar.
Keep in mind that fsevents should only be required in OS X systems.
I played around with it for an hour or two and, for the most part, sane seemed to be able to take the place of chokidar without issue. Would something like this be considered?

@SpaceK33z
Copy link
Member

SpaceK33z commented Dec 8, 2016

I think we can definitely consider this. In #26 @gullitmiranda suggest to use Gaze. Have you also looked into that (I have no idea which is "better")?

Do you think you would be able to make a proof of concept using sane (or Gaze)? I could test it on some projects of mine, and later we could ask for more people to test it. It won't be an easy change, but might be worth it in the long run.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

I had looked at gaze a while back, but I remember that the version strings of its sub-dependencies were very rigid, so it didn't dedupe particularly well (i.e. you would have lodash@4.16.x from one of the dependencies of gaze and lodash@4.17.x somewhere else). Admittedly, that is not a very good reason to pick one library over the other.
I will attempt to make a proof of concept using sane.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

I have this mostly complete, but there are a couple of tests that are not passing. sane seems to differ from chokidar in that writing to a file that did not exist will fire an add and a change event instead of just an add. I opened an issue to ask about this, but I do not understand how to apply the response.

@SpaceK33z
Copy link
Member

Maybe you could just not listen on the add event? I don't know if this would work in all cases, but if change always fires when add is fired, that might work.

@SpaceK33z
Copy link
Member

SpaceK33z commented Dec 14, 2016

Btw, when you have fixed this, we could ask some people from create-react-app to help with testing. I think it was @gaearon who was asking about if it was possible to use watchman in webpack. sane supports watchman, so that would fix it.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

It was. Actually, that tweet is kinda the reason I decided to try this.

@gaearon
Copy link

gaearon commented Dec 14, 2016

Yeah I'd be happy to test it.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

wtgtybhertgeghgtwtg commented Dec 14, 2016

Maybe you could just not listen on the add event? I don't know if this would work in all cases, but if change always fires when add is fired, that might work.

The issue isn't so much the add event as it is the change event that follows some milliseconds later. Some tests assert that there will be a certain number of change events fired, whiles others assert that no change event is fired.
On the plus side, commenting out these types of checks makes everything pass, so it seems that this is the only problem.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

Proof of Concept: #41.

@curioussavage
Copy link

Was this proposal abandoned?

@alexander-akait
Copy link
Member

Yes

@felixfbecker
Copy link

Any reason why?

@MohamedLamineAllal
Copy link

yes for what reason?

@rauchg
Copy link

rauchg commented Dec 17, 2018

I'd love to re-examine this. A big chunk of time in next is spent installing chokidar (specifically fsvents, but as mentioned above the dependency tree is gigantic). In order to "webpack webpack", we need to exclude chokidar in https://github.com/zeit/ncc.

Furthermore, it doesn't seem like chokidar's stated benefits in its README are valid today according to a libuv's maintainer:

libuv/libuv#2096 (comment)

@alexander-akait
Copy link
Member

The chokidar tool is very stable compared with sane, not sure we really need migrate on other solution, somebody can describe why we need do this?

@lovetingyuan
Copy link

while we do not use both chokidar and sane, just use fs.watch 🙂

@alexander-akait
Copy link
Member

@lovetingyuan fs.watch is very low api level and contains some incompatibilities between os

@lovetingyuan
Copy link

@evilebottnawi yes, but the new version of watchpack has removed chokidar and just use fs.watch directly.

@wtgtybhertgeghgtwtg
Copy link
Contributor Author

That would close this issue.

@chenxsan
Copy link
Member

Seems jest will replace sane with chokidar then jestjs/jest#10048

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants