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

Make fs.watch recursive on Windows #375

Closed
jorangreef opened this issue Jan 14, 2015 · 11 comments
Closed

Make fs.watch recursive on Windows #375

jorangreef opened this issue Jan 14, 2015 · 11 comments
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. windows Issues and PRs related to the Windows platform.

Comments

@jorangreef
Copy link
Contributor

I spotted the recursive watch support for OS X in the io.js changelog.

There's a pull request for libuv at joyent/libuv#1473 to add recursive watch support for Windows but it has been sitting for a few months.

Is there anyone here who could help to get that through?

If we had recursive watch support for OS X and Windows, then Linux could be handled using a series of inotify watchers (we can't do this on Windows because it would lock every watch directory or parent thereof), and we could get a decent cross-platform watch implementation working on top of io.js.

@KyleAMathews
Copy link

Most people writing cross-os tools don't rely on core fs.watch but various fs.watch replacements/supplements such as https://github.com/paulmillr/chokidar

@bnoordhuis
Copy link
Member

If we had recursive watch support for OS X and Windows, then Linux could be handled using a series of inotify watchers

That won't really work - or at least, work well - due to the relatively low per-user inotify limit. It's something you can tweak through a sysctl but most people leave it at the default (for good reasons, having too many watchers seriously impacts performance.)

@jorangreef
Copy link
Contributor Author

It would help to have the recursive support for Windows at least. For Linux there is no choice but to make the most of inotify as far as possible (only watching directories, not files) and then fall back to polling when inotify limits are reached. That's better than just polling to begin with. But for Windows, there's no way to use fs.watch at all at present (unless it has recursive support) because of how it locks every watched directory.

@Fishrock123 Fishrock123 added the windows Issues and PRs related to the Windows platform. label Jan 22, 2015
@Fishrock123 Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Feb 6, 2015
@bpasero
Copy link
Contributor

bpasero commented Feb 6, 2015

+1 to expose the recursive option on windows. I really want to benefit from the native support of recursing and not rely on some custom solution that has to a) lock every child folder and b) consume lots of CPU/Memory while doing so.

Please please please add this flag for windows.

@bnoordhuis
Copy link
Member

@bpasero io.js accepts pull requests, you know. :-)

Jest aside, I personally don't object to a recursive flag. The pitfalls need to be called out in the documentation, though.

@bpasero
Copy link
Contributor

bpasero commented Feb 6, 2015

@bnoordhuis whats wrong with joyent/libuv#1473 ?

@bnoordhuis
Copy link
Member

@saghul already called it out, it breaks the ABI. It's also possibly stale by now. Someone needs to pick it up and do the work of getting the necessary changes landed in libuv and io.js.

My careful use of the word 'someone' is a hint that it won't be me. :-)

@Qard
Copy link
Member

Qard commented Feb 6, 2015

I'm also interested in this for https://github.com/qard/onchange. I don't really do Windows dev though, so I'm not sure I'd be able to put together a decent PR for this. I'm hoping someone better at Windows figures this out before I can take the time to look into it.

@bpasero
Copy link
Contributor

bpasero commented Feb 11, 2015

@bnoordhuis I created libuv/libuv#198. I think this report can be closed as it seems to be a libuv only thing?

@Fishrock123 Fishrock123 added the libuv Issues and PRs related to the libuv dependency or the uv binding. label May 26, 2015
@brendanashworth brendanashworth added the feature request Issues that request new features to be added to Node.js. label Jun 5, 2015
@Qard
Copy link
Member

Qard commented Aug 14, 2015

This is in libuv as of 1.7.0, so it should get in when #2310 is merged.

thefourtheye added a commit that referenced this issue Sep 4, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
@silverwind
Copy link
Contributor

Fixed by a338eb3

thefourtheye added a commit that referenced this issue Sep 5, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
thefourtheye added a commit that referenced this issue Sep 5, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
thefourtheye added a commit that referenced this issue Sep 6, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
thefourtheye added a commit that referenced this issue Sep 6, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
thefourtheye added a commit that referenced this issue Sep 6, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
thefourtheye added a commit that referenced this issue Sep 6, 2015
Recursive file watching is supported by libuv since 1.7.0. Refer
https://github.com/nodejs/node/blob/master/deps/uv/ChangeLog#L126. This
patch notes that in the docs and enables testing this feature. It also
adds proper TAP plugin parsable message for other platforms.

PR-URL: #2649
Fixes: #375
Reviewed-By: rvagg - Rod Vagg <rod@vagg.org>
Reviewed-By: silverwind - Roman Reiss <me@silverwind.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system. libuv Issues and PRs related to the libuv dependency or the uv binding. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

No branches or pull requests

8 participants