-
Notifications
You must be signed in to change notification settings - Fork 24
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
Tidy up overrides and add watch API #227
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks great, just added two small requests.
Also, are there any breaking changes in this PR? (I guess the behaviour of watch()
changes.)
I've done the changes you asked for. There's no breaking changes AFAIK here, just a what I would call a "fix" for #89 to support watch. EDIT: I actually was reading https://nodejs.org/docs/latest/api/fs.html#fs_fs_unwatchfile_filename_listener and now added a method which throws an error when that is called - since we don't track watchers to be able to call |
Realized my mistake in my latest push that broke the build in switching to Set. This should be good to merge now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM
@LukeSheard Did you test if it throws an exception when an invalid is provided |
Following reading #89 I thought I'd PR two things:
FS.FSWatcher
which calls all the watchers we get back from the underlying filesystems.