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

Bundle fsevents in Node.js core #17

Open
mcollina opened this issue Feb 8, 2019 · 9 comments
Open

Bundle fsevents in Node.js core #17

mcollina opened this issue Feb 8, 2019 · 9 comments

Comments

@mcollina
Copy link
Member

mcollina commented Feb 8, 2019

One of the must-have modules in the ecosystem is https://www.npmjs.com/package/fsevents. fsevents is needed if you need to watch files on Mac OS X, as our current file watching strategy is inadeguate and unreliable.

Should we ship fsevents implementation in Node.js core?

cc @pipobscure

@MylesBorins
Copy link

I believe the bits that are done byfsevents make the most sense in libuv, but the team didn't seem to agree when I opened an issue

libuv/libuv#2096

Does the OSX specific implementation belong in core?

@mcollina
Copy link
Member Author

mcollina commented Feb 8, 2019

Reading on that topic, is the node core implementation powerful enough already? In other terms, is fsevents now redundant?

@refack
Copy link

refack commented Feb 9, 2019

In other terms, is fsevents now redundant?

It does seem so. To some level...
Ref: https://mobile.twitter.com/refack/status/1073685596298772483
tl;dr the chokidar test suite passes almost all test with native node@12.

@mcollina
Copy link
Member Author

mcollina commented Feb 9, 2019

@refack which tests are not passing? It could be a good goal to have chokidar without fsevents in Node 12.

@pipobscure
Copy link

Chokidar is code by evolution and could benefit from cleanup & refactor. As part of that chokidar could be subsumed into core.

FSEvents was also code by evolution. Except it has recently benefited from a refactor & cleanup, which has significantly simplyfies the API & the implementation.

Either or both could be in core. But I would not put in either.

—-

Now as to the question of “is fs.watch powerful enough now”.

There’s orders of magnitude difference in performance between fsevents and fs.watch even now.

Due to loads of improvements there’s nothing that can’t be done slowly using fs.watch.

@mcollina
Copy link
Member Author

Either or both could be in core. But I would not put in either.

One of the major use cases for Node.js is to support the tools that are used to build modern web apps. In a significant amount of use cases, this includes watching for changes in the filesystem. As things stand now, the Node.js runtime was not able to provide a performant implementation on Mac OS X.
fsevents did.

I'd like to explore a scenario where such a critical functionality would not require a native addon. Native addons could pose a security risk in a lot of cases, and avoiding them altogether is a last resort option. I was recently discussing if it was feasibile to add security policies to Node.js - the first thing that would not be possible to support are native addons.

I think it's a good question to ask. Can you articulate on why you would not put in either?

@pipobscure
Copy link

I guess we come at it from a different perspective. My main use-case has always been micro-service apis. Most of those run on anything but MacOs.

If I were to share your perspective and assume it to be the majority use-case then yes, it should go into core.

Whatever the conclusion. I’d be more than happy to help migrate fsevents into core if that’s wanted. By now I’m quite comfortable with the dichotomy between OSX run-loops and libuv run-loops required to make this work. It’s a bit messy though.

And that might be the biggest reason why I wouldn’t put it into core. It’s fairly messy. And applies only to a single OS. Is it worth “polluting” core with that?

@ljharb
Copy link
Member

ljharb commented Feb 10, 2019

There’s tons of “pollution” in core to handle windows, which is just one OS. I’d say the real question is, is it worth not meeting the needs of an entire supported OS in order to meet some kind of “cleanliness” bar?

@boneskull
Copy link
Collaborator

Is it worth “polluting” core with that?

unequivocally, YES

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

No branches or pull requests

6 participants