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.

Sign up for GitHub

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

Jump to bottom

fs.watch should throw an exception if recursive is not supported #29901

Closed
exx8 opened this issue Oct 9, 2019 · 3 comments
Closed

fs.watch should throw an exception if recursive is not supported #29901

exx8 opened this issue Oct 9, 2019 · 3 comments
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@exx8
Copy link

exx8 commented Oct 9, 2019 •
edited
Loading

It has been discussed in the past, that recursion watch is not implementable with good performances in Linux . And it is actually documented, But currently, the fs.watch just ignores this option without signalling the user that the recursive has been ignored. I truly believe that an exception should be thrown if recursive is ignored, as it might cause unexpected bugs while cross-platforming an app or a script, And probably most users wouldn't imagine that such incapability exists, and probably won't look it up.
Of course, this is a breaking change.
@sam-github
Copy link
Contributor

Seems reasonable to me.

@exx8
Copy link
Author

exx8 commented Oct 9, 2019

Okay, I believe I should wait for more approvals?
Then I'll be more than happy for this to be my first contribution to the project.

@BridgeAR
Copy link
Member

BridgeAR commented Oct 9, 2019

@exx8 feel free to open a PR with the necessary changes. Such changes are rarely discussed in depth beforehand and the PR itself should be sufficient.

@exx8 exx8 mentioned this issue Oct 12, 2019
Closed
4 tasks
exx8 pushed a commit to exx8/node that referenced this issue Oct 12, 2019
fs: watch signals for recursive incompatibility.
f71e6fb 
this pull request makes fs.watch throw exception,
whenever it is used in an incompatible platform.
for this change following changes were made to api:

1.a new error type has been introduced.
2.fs.watch has been changed accordingly.

Users who use recursive  on
non-windows and osx platforms,
will face a new exception.
for this reason,
 it's a breaking change.

Fixes: nodejs#29901
@Fishrock123 Fishrock123 added the fs Issues and PRs related to the fs subsystem / file system. label Oct 14, 2019
Trott pushed a commit to exx8/node that referenced this issue Jan 24, 2020
@Trott
fs: watch signals for recursive incompatibility
fa5984b 
this pull request makes fs.watch throw exception,
whenever it is used in an incompatible platform.
for this change following changes were made to api:

1.a new error type has been introduced.
2.fs.watch has been changed accordingly.

Users who use recursive  on
non-windows and osx platforms,
will face a new exception.
for this reason,
 it's a breaking change.

Fixes: nodejs#29901
@Trott Trott closed this as completed in 67e067e Jan 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@sam-github @Fishrock123 @exx8 @BridgeAR