Simplify/Reduce API

[pipobscure]

Changes:

1. Reduce the API
2. Single thread for all watchers
3. Eliminate extraneous stat on every change event
4. Increased testing and coverage
5. Eliminated tap as a dependency making it lighter weight for development

The API now consists of:

fsevents.start(path : string, handler : function) : function
fsevents.getInfo(path: string, flags: number) : InfoObject
fsevents.constants : { [ name : string ] : number }

This is sufficient for what chokidar uses, and allows eliminating the extra stat call on every event.

In addition this now uses the napi threading routines, and only has a single thread for all watchers making it significantly cheaper.

[paulmillr]

Nice!

[paulmillr]

Just clarifying — after the backport, 8.x users would be fine?

[pipobscure]

Yes. Once the backport is done an released it will work with all future versions of 8.x

All versions of v8.x before that won’t. Neither will versions before 10.6 on the v10.x line.

[bnoordhuis]

@pipobscure Nice work! As a bit of expectation management, I probably won't be able to do an in-depth review until next week.

[pipobscure]

Required Node (v8.x) PR: nodejs/node#25002

[paulmillr]

@pipobscure that got landed now

[pipobscure]

ToDo: Mark this as NAPI_VERSION 4

[pipobscure]

@bnoordhuis can you do a review when you get a chance?

[SimenB]

I mean that fsevents won't, from what I've understood, support node 6 and node 12 at the same time for any release line.

[paulmillr]

Can we merge it? Why does it take like forever to get PRs merged here? @bnoordhuis can you give me maintainers rights?

[paulmillr]

@SimenB neither will node.js. End of life for v6 would happen when v12 would be out. No issues here.

[pipobscure]

I’m waiting for an actual code-review as a matter of principle. That’s why I released it to npm as fsevents@napi already, but haven’t merged.

But on the other hand, if it doesn’t get a review by the end of March, then I’ll merge. After all, things need to move on.

[bnoordhuis]

I have very limited time at the moment because of $life. I'd like someone else to review it.

[pipobscure]

Thanks @bnoordhuis you’ve given more than enough of your time over the years.

[paulmillr]

@bnoordhuis that's perfectly fine — could you hand over the maintainer rights to me?

[pipobscure]

I wonder whether it would make sense to move the project from strongloop. I bet I can get Bloomberg to take it on if we want to go that route.

[pipobscure]

@paulmillr if you just do a review, then I can merge it. I’m just a big believer in code-reviews.

[bnoordhuis]

could you hand over the maintainer rights to me?

@paulmillr I tried upgrading @pipobscure and @es128 to maintainers a while ago but for some reason it doesn't work, the admin page times out when I do.

I'm happy to transfer it though if that's what @pipobscure wants.

[paulmillr]

I need:

• npm owner add paulmillr fsevents — NPM rights
• GitHub access. This can only be done if you have admin rights in Strong Loop fsevents. If you don't, let us know who does. We can also have the repo transferred to pipobscure account if that would be simpler.

[pipobscure]

I’ll check and get back to you.

[pipobscure]

Ok, let's move it to its own Org (fsevents) that I've just setup.

[bnoordhuis]

This can only be done if you have admin rights in Strong Loop fsevents.

To be clear, I do - I can initiate the transfer - but some GH bug prevents me from modifying member permissions. I speculate it's because this is effectively a mothballed org.

[paulmillr]

@bnoordhuis let's do the transfer.

[bnoordhuis]

Done.

[paulmillr]

Thank you!

[paulmillr]

@pipobscure could you give me the rights now?

[pipobscure]

@paulmillr Done (I also added you to npm)

[paulmillr]

The PR Looks great from my end. We'll need to add v8 support once the LTS is out.

Haven't reviewed C code though since i'm not that proficient with it.

[pipobscure]

How about we merge, and I'll get onto v8 support as soon as that's released.
The latest v8.15.1 is still the old version (released in Feb). The next one has it, it's just a question of when that ships and whether as v8.15.2 or v8.16.0

After that I'll try to update the chokidar PR to bring that top v2 as well.

[realityking]

More or less crazy idea: what do you think about building two versions of the binary, one N-API version 4 (current code) and one with N-API version 3 (or even 1) using the old libuv code instead of the new N-API version 4 code. Since any Node.js version that doesn't support N-API 4 should have the current libuv version it'd be forward compatible.

The reason I bring this up is that I'm afraid adoption of chokidar 2 will suffer if it required Node.js 8.16. Most project will only go up to 8.0 or maybe 8.9 (first LTS).

I'm happy to try my hands on it if you agree it's generally a good idea.

[paulmillr]

Folks who want to use ancient nodejs versions are free to use chokidar 1 and 2. We’ve been supporting node 0.10 ever since.

Chokidar 3 will drop support for anything but 8.16+. It needs to be understood that the nodejsv8 would reach end of life in just eight months.

[pipobscure]

@realityking the problem isn’t a missing/changed API in libuv, but that lubuv is missing a key feature.

You’d have to implement:

napi_create_threadsafe_function and napi_call_threadsafe_function

[pipobscure]

@realityking also I don’t quite understand your point though.

Versions of v8.x lower than v8.15.1 are currently already unsupported. If you file an issue against one of those the nodejs team would just tell you to update to v8.15.1

As soon as the next version of the v8.x line is release, that version becomes the only supported one. If a busuiness refuses to update within a major, that could be considered reckless.

So why would that hamper adoption?

[realityking]

@pipobscure You're correct in terms of support by Node.js itself. But when you look at the npm ecosystem, most packages raise their support to >=8.0 when it time to drop support for Node.js 6. I'm not sure if packages will be willing to go to >=8.16 < 9.0 || >=10.16 < 11.0 || >= 11.8 - quite bit more restrictive.

[pipobscure]

I agree @realityking, but that’s why it’s a major version switch of fsevents and a double major in terms of chokidar.

The alternative is to never update/change anything ever. And that can’t be good for the ecosystem either. If that was done in medicine we’d still be using blood-letting, leeches and the like.

I think we found a good path forward that should bring as many people along as possible while also not neglecting innovation.

[realityking]

Don't get me wrong, @pipobscure, I'm a massive fan of the switch to N-API. The question was merely if a patch to make it compatible with all Node.js version that support N-API would be accepted.

[pipobscure]

Yes it would. If anyone wants to give that a go, I’d be more than happy.

You need to realise though that N-API is not a monolith. So we require v4 of N-API. You’d have to somehow backport that (as was done for the v8.x branch of node and pray that you can do it in a way that doesn’ then explode. That’s actually pretty difficult to do.

That being said: there are many people out there more brilliant than I, if one of them wants to give this a go, I’d be ecstatic to accept that PR.