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.

Sign up for GitHub

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

Jump to bottom

remove fsevents #130

Merged
merged 2 commits into from Sep 26, 2018
Merged

remove fsevents #130

merged 2 commits into from Sep 26, 2018

Conversation

stefanpenner
Copy link
Collaborator

No description provided.

@stefanpenner
Copy link
Collaborator Author

Spoke with @amasad in person yesterday this can be safely removed, as the original use-case (from jest) has taken a different approach.

If users are having FS watching issues, OSX's filewatching is pretty sad, we strongly recommend they utilize watchman.

@stefanpenner stefanpenner merged commit b4c4f03 into master Sep 26, 2018
@stefanpenner stefanpenner deleted the remove-fs-events branch September 26, 2018 18:43
rwjblue
rwjblue reviewed Sep 26, 2018
View reviewed changes
Copy link

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@stefanpenner
Copy link
Collaborator Author

released as v4.0.0 🎉

@stefanpenner stefanpenner mentioned this pull request Sep 26, 2018
Merged
@wtgtybhertgeghgtwtg
Copy link
Collaborator

Is there a reason fsevents is left as an optional dependency?

@rwjblue
Copy link

rwjblue commented Sep 27, 2018

Seems like an oversight, mind sending a PR?

@wtgtybhertgeghgtwtg wtgtybhertgeghgtwtg mentioned this pull request Sep 27, 2018
Merged
@stefanpenner
Copy link
Collaborator Author

stefanpenner commented Sep 27, 2018 •
edited

@wtgtybhertgeghgtwtg thanks, I released your fix as v4.0.1 🎉

@amasad
Copy link
Owner

amasad commented Sep 27, 2018

Great--thanks @stefanpenner et al

@SimenB
Copy link

SimenB commented Sep 30, 2018

the original use-case (from jest) has taken a different approach.

Question, what is that approach? Jest currently uses the FSEventsWatcher removed in this PR: https://github.com/facebook/jest/blob/d0e627cb974af0c85c93befb914a45f8c57e5bac/packages/jest-haste-map/src/index.js#L684-L689

@SimenB
Copy link

SimenB commented Oct 30, 2018

ping @stefanpenner.

@SimenB SimenB mentioned this pull request Nov 2, 2018
Closed
@SimenB SimenB mentioned this pull request Mar 7, 2019
Merged
@SimenB
Copy link

SimenB commented Aug 20, 2019

Any chance of reverting this? We have to vendor the previous solution in Jest (due to jestjs/jest#8088, jestjs/jest#8258). I don't know what the motivation for removing it was, but v2 of fsevents uses napi, so should be less of a pain for consumers if that was part of it

@stefanpenner
Copy link
Collaborator Author

I’ll take a look once I return of vacation. If you don’t hear from me by next week, it may have gotten dropped. Feel free to reping

@SimenB
Copy link

SimenB commented Sep 2, 2019

@stefanpenner ping 😀

facebook-github-bot pushed a commit to facebook/flow that referenced this pull request Sep 18, 2019
@mroch @facebook-github-bot
[tool] update sane dep
e5ad207 
Summary: sane@2 added fsevents support. sane@3 bumped the minimum node version to 6.0, same as ours. sane@4 removed fsevents support, but that seems to be controversial, so excluding it for now: amasad/sane#130

Reviewed By: gabelevi

Differential Revision: D17425813

fbshipit-source-id: 426aee1710cd84a5bf27ddbfc8c5977b33e053fd
@SimenB
Copy link

SimenB commented Oct 31, 2019

@stefanpenner any news here?

@SimenB SimenB mentioned this pull request Nov 16, 2019
Closed
@SimenB
Copy link

SimenB commented Jan 2, 2020

@stefanpenner 2020 ping 🎆

@SimenB SimenB mentioned this pull request May 11, 2020
Open
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rwjblue rwjblue rwjblue left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@stefanpenner @wtgtybhertgeghgtwtg @rwjblue @amasad @SimenB