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

Update fsevents dependency to use napi #774

Closed
wants to merge 1 commit into from

Conversation

pipobscure
Copy link
Contributor

@pipobscure pipobscure commented Nov 10, 2018

This updated the fsevents dependency to use the napi tagged version (currently 2.0.2-pre-1)

There are a few API changes, and support for node-versions below v8 has been dropped, whence the major version bump. (v8.x support is not there currently but will become available through nodejs/node#25002)

The big advantage of this is that fsevents is now using N-API which means a single build works with all versions of node. No more node-pre-gyp downloading specific versions and upgrading with every new node release.

In addition fsevents now uses a single runloop/thread to listen for events which makes it significantly cheaper; and it reduces the API to just handle the native stuff, which is all chokidar uses which leads to eliminating a stat call per event, again making it a lot cheaper.

I have also tested the regular binary against the upcoming version of electron (v4.0.0-beta7) and it works out of the box. So this should also eliminate some of the pain associated with that.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.2%) to 85.028% when pulling f3f33a3 on pipobscure:UpdateFSEvents into 0a46e6f on paulmillr:master.

@coveralls
Copy link

coveralls commented Nov 10, 2018

Coverage Status

Coverage decreased (-20.9%) to 75.317% when pulling fecf799 on pipobscure:UpdateFSEvents into 0a46e6f on paulmillr:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-11.2%) to 85.028% when pulling f3f33a3 on pipobscure:UpdateFSEvents into 0a46e6f on paulmillr:master.

@pipobscure
Copy link
Contributor Author

@paulmillr this is the current version to experiment with fsevents/fsevents#247

There are 2 test failures where the spy gets called twice rather than once and they are special-cased with osXFsWatch so I'm not quite sure whether this is real or not, but whatever the case, I'll need help figuring those out.

This should bring us much closer to using NAPI as of April, when we have the backport of the things fsevents needs to v8.x (currently only v10.6+). (per nodejs/node#24249 )

My thought is to use the April EOL of v6 to release this all and limit it's compatibility to v8.x+

Comments?

@pipobscure pipobscure changed the title Update FSEvents dependency to 2.0.0 Update fsevents dependency to use napi Dec 29, 2018
@paulmillr
Copy link
Owner

Yeah, that sounds great.

Do you have any idea why was the test coverage decreased?

@pipobscure
Copy link
Contributor Author

Sadly I have not. I’m just happy that it actually runs through all the tests. 😀

@paulmillr
Copy link
Owner

Merged in master.

@paulmillr paulmillr closed this Mar 22, 2019
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 this pull request may close these issues.

None yet

4 participants