fs.watch throws exception when recursive is used in incompatible platform by exx8 · Pull Request #29947 · nodejs/node · GitHub

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:

A new error type has been introduced.
fs.watch has been changed accordingly.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Solves #29901
This is a breaking change.

Okay I had to alter the first commit so it will be compatible to the format of the project. I believe that the PR is ready now.

The PR has a bit changed, it now uses a new more generic error, ERR_FEATURE_UNAVAILABLE_ON_PLATFORM, which I believe it's more suitable, as like was demonstrated in the conversation with @addaleax , handles more cases (such as fs.chmod, which I hope in the future will also throw this exception for example in windows).

Left some comments. Thanks for the PR!

Nit: this could be simplified as:

E('ERR_FEATURE_UNAVAILABLE_ON_PLATFORM',
  'The feature %s is unavailable on the current platform' +
    ', which is being used to run Node.js',
  TypeError);

(It's a semi standard to use that pattern if the error is not too complex)

LGTM with the comments incorporated.

I switched from try and catch to built in function & extracted the old logic so it will be more neat. I also reformatted the errors.

Still LGTM

@nodejs/tsc

LGTM

Just a nit... I would just make this more generic like ERR_NOT_IMPLEMENTED or ERR_NOT_AVAILABLE, but it's not critical

I think there is a great value of knowing this error occurs only on one platform, and that it isn't something that node.js itself doesn't support.

CI: https://ci.nodejs.org/job/node-test-pull-request/26089/

ok I can see that some tests fail. Can I contribute to fix it?

CI: https://ci.nodejs.org/job/node-test-pull-request/26377/

@exx8 Sorry, it’s been a while and the old CI results are no longer available. I’ve started a new one, though.

Generally, feel free to ping us if you feel like a PR is stalling out somehow.

@exx8 Sorry, it’s been a while and the old CI results are no longer available. I’ve started a new one, though.

Generally, feel free to ping us if you feel like a PR is stalling out somehow.

It seems that there is a merge conflict now (according to the build:

18:37:55 M	lib/internal/errors.js
18:37:55 Falling back to patching base and 3-way merge...
18:37:55 Auto-merging lib/internal/errors.js
18:37:55 CONFLICT (content): Merge conflict in lib/internal/errors.js
18:37:55 error: Failed to merge in the changes.

) or it might be a false alarm (as github says there aren't).
Do you want me to fix it?
Will it require everyone to re-approve the PR?
@addaleax

) or it might be a false alarm (as github says there aren't).

I think the issue is that one of the 14 commits in this PR has a merge conflict, but not the PR as a whole (i.e. one of the later commits removed the merge conflict again).

Do you want me to fix it?

It would be helpful if you could rebase this (not git merge) against master, so that we can run CI the usual way. If you’re not feeling comfortable with that, you can also let somebody else do that.

Will it require everyone to re-approve the PR?

If the contents of the PR stay the same apart from resolving merge conflicts, that should be okay.

Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

Rebased to eliminate conflict.

@Trott seems like there's a linter issue now?

CI: https://ci.nodejs.org/job/node-test-pull-request/28610/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2241/

CI: https://ci.nodejs.org/job/node-test-pull-request/28613/

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: #29901 PR-URL: #29947 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Saúl Ibarra Corretgé <saghul@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Landed in 67e067e.

Thanks for the contribution! 🎉

This makes sure this function stays backwards compatible in case it's accessed through the binding directly. Refs: #29947 (comment) Backport-PR-URL: #31431 PR-URL: #30858 Refs: #30767 Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: David Carlier <devnexen@gmail.com> Reviewed-By: Yongsheng Zhang <zyszys98@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Use `chokidar` to track file system changes as soon it will throw an error if the platform does not support `{ recursive: true }`. Fix also an error where the built file was not being deleted when the source was removed. - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Fix react-native-community#989 Ref: nodejs/node#29947

Use `chokidar` to track file system changes as soon it will throw an error if the platform does not support `{ recursive: true }`. Fix also an error where the built file was not being deleted when the source was removed. - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - Fix #989 Ref: nodejs/node#29947

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Oct 12, 2019

mscdex reviewed Oct 12, 2019

View reviewed changes

Comment thread test/parallel/test-fs-watch-recursive.js Outdated

exx8 force-pushed the recursive_err branch 2 times, most recently from f39ce99 to 76c5f11 Compare October 12, 2019 19:41

addaleax reviewed Oct 12, 2019

View reviewed changes

Comment thread doc/api/errors.md Outdated

Comment thread doc/api/fs.md Outdated

Comment thread lib/internal/errors.js Outdated

bnoordhuis reviewed Oct 14, 2019

View reviewed changes

Comment thread doc/api/fs.md Outdated

Comment thread doc/api/fs.md Outdated

Comment thread lib/internal/errors.js Outdated

Comment thread test/parallel/test-fs-watch-recursive.js Outdated

BridgeAR reviewed Oct 14, 2019

View reviewed changes

BridgeAR approved these changes Oct 14, 2019

View reviewed changes

BridgeAR reviewed Oct 15, 2019

View reviewed changes

Comment thread test/parallel/test-fs-watch-recursive.js Outdated

Trott added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 16, 2019

mcollina approved these changes Oct 16, 2019

View reviewed changes

jasnell reviewed Oct 17, 2019

View reviewed changes

jasnell approved these changes Oct 17, 2019

View reviewed changes

addaleax approved these changes Oct 17, 2019

View reviewed changes

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Oct 17, 2019

saghul approved these changes Oct 18, 2019

View reviewed changes

lholmquist approved these changes Oct 21, 2019

View reviewed changes

Trott approved these changes Dec 6, 2019

View reviewed changes

exx8 and others added 4 commits January 24, 2020 05:56

Update doc/api/fs.md

b0fc0eb

Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

Update lib/internal/errors.js

cb24297

Co-Authored-By: Ben Noordhuis <info@bnoordhuis.nl>

reformatting the error definition, and refactor the test

7e23cef

updating test.

120a471

Trott force-pushed the recursive_err branch from 22e67c2 to 120a471 Compare January 24, 2020 13:58

This comment has been minimized.

Sign in to view

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Jan 24, 2020

richardlau reviewed Jan 24, 2020

View reviewed changes

Comment thread doc/api/errors.md Outdated

Trott added 2 commits January 24, 2020 07:45

fixup! updating test.

632c0e1

fixup! fixup! updating test.

b3db344

Trott closed this Jan 25, 2020

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Jan 25, 2020

molant mentioned this pull request Feb 19, 2020

yarn watch doesn't get changes from all files on Linux and Windows react-native-community/cli#989

Closed

molant mentioned this pull request Feb 19, 2020

fix: make watch work in all platform react-native-community/cli#991

Merged

BethGriggs mentioned this pull request Mar 10, 2020

2020-04-21, Version 14.0.0 (Current) #32181

Merged

intech mentioned this pull request Sep 9, 2020

Issue with hot reload w/ moleculer-runner on node 14 moleculerjs/moleculer#810

Closed

coderabbitai Bot mentioned this pull request Aug 1, 2026

chore(mcp-server): restart the dev server only on real source changes vinikjkkj/zapo#224

Merged

coderabbitai Bot mentioned this pull request Aug 10, 2026

Full TypeScript migration, one-clock smoothness, single-controller guard jtn0123/squares_controller#6

Merged

4 tasks

fs.watch throws exception when recursive is used in incompatible platform by exx8 · Pull Request #29947 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs.watch throws exception when recursive is used in incompatible platform - #29947

fs.watch throws exception when recursive is used in incompatible platform#29947exx8 wants to merge 15 commits intonodejs:masternodejs/node:masterfrom exx8:recursive_errexx8/node:recursive_errCopy head branch name to clipboard

Conversation

exx8 commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Uh oh!

exx8 commented Oct 12, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

exx8 commented Oct 13, 2019

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BridgeAR Oct 14, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

exx8 commented Oct 15, 2019

Uh oh!

Uh oh!

BridgeAR commented Oct 15, 2019

Uh oh!

Trott commented Oct 16, 2019

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

exx8 Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 17, 2019

Uh oh!

exx8 commented Oct 18, 2019

Uh oh!

nodejs-github-bot commented Nov 5, 2019

Uh oh!

addaleax commented Nov 5, 2019

Uh oh!

exx8 commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Nov 6, 2019

Uh oh!

Trott commented Jan 24, 2020

Uh oh!

This comment has been minimized.

BridgeAR commented Jan 24, 2020

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 24, 2020 • edited by Trott Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jan 24, 2020

Uh oh!

Trott commented Jan 25, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

fs.watch throws exception when recursive is used in incompatible platform#29947
exx8 wants to merge 15 commits into
nodejs:masterfrom
exx8:recursive_err

exx8 commented Oct 12, 2019 •
edited
Loading

exx8 commented Nov 6, 2019 •
edited
Loading

nodejs-github-bot commented Jan 24, 2020 •
edited by Trott
Loading