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

exx8 · 2019-10-12T16:37:02Z

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.

test/parallel/test-fs-watch-recursive.js

exx8 · 2019-10-12T19:54:48Z

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.

doc/api/errors.md

doc/api/fs.md

lib/internal/errors.js

exx8 · 2019-10-13T07:29:02Z

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).

bnoordhuis

Left some comments. Thanks for the PR!

doc/api/fs.md

lib/internal/errors.js

test/parallel/test-fs-watch-recursive.js

BridgeAR · 2019-10-14T12:52:51Z

lib/internal/errors.js

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


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)

test/parallel/test-fs-watch-recursive.js

BridgeAR

LGTM with the comments incorporated.

exx8 · 2019-10-15T12:16:26Z

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.

test/parallel/test-fs-watch-recursive.js

BridgeAR · 2019-10-15T14:57:44Z

Still LGTM

Trott · 2019-10-16T06:06:27Z

@nodejs/tsc

mcollina

LGTM

jasnell · 2019-10-17T14:46:41Z

doc/api/errors.md

@@ -2352,6 +2352,12 @@ The `--entry-type=...` flag is not compatible with the Node.js REPL.
 Used when an [ES Module][] loader hook specifies `format: 'dynamic'` but does
 not provide a `dynamicInstantiate` hook.

+<a id="ERR_FEATURE_UNAVAILABLE_ON_PLATFORM"></a>
+#### ERR_FEATURE_UNAVAILABLE_ON_PLATFORM


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.

nodejs-github-bot · 2019-10-17T20:20:10Z

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

exx8 · 2019-10-18T08:13:08Z

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

nodejs-github-bot · 2019-11-05T22:23:37Z

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

addaleax · 2019-11-05T22:24:13Z

@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 · 2019-11-06T08:36:36Z

@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

addaleax · 2019-11-06T11:19:14Z

) 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>

Trott · 2020-01-24T13:59:00Z

Rebased to eliminate conflict.

BridgeAR · 2020-01-24T14:09:42Z

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

doc/api/errors.md

nodejs-github-bot · 2020-01-24T15:50:29Z

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

nodejs-github-bot · 2020-01-24T16:48:06Z

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>

Trott · 2020-01-25T04:58:11Z

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

test/parallel/test-fs-watch-recursive.js Outdated Show resolved Hide resolved

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

doc/api/errors.md Outdated Show resolved Hide resolved

doc/api/fs.md Outdated Show resolved Hide resolved

lib/internal/errors.js Outdated Show resolved Hide resolved

bnoordhuis reviewed Oct 14, 2019

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

doc/api/fs.md Outdated Show resolved Hide resolved

lib/internal/errors.js Outdated Show resolved Hide resolved

test/parallel/test-fs-watch-recursive.js Outdated Show resolved Hide resolved

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

test/parallel/test-fs-watch-recursive.js Show resolved Hide resolved

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 pending requests for changes, 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

Eran Levin and others added 6 commits January 24, 2020 05:56

fix to documentation

194ca4d

Update doc/api/fs.md

a0b5dd5

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

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 pending requests for changes, and a CI started. label Jan 24, 2020

richardlau reviewed Jan 24, 2020

View reviewed changes

doc/api/errors.md Outdated Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

exx8 commented Oct 12, 2019 •

edited

Loading

exx8 commented Oct 12, 2019

exx8 commented Oct 13, 2019

bnoordhuis left a comment

BridgeAR Oct 14, 2019

BridgeAR left a comment

exx8 commented Oct 15, 2019

BridgeAR commented Oct 15, 2019

Trott commented Oct 16, 2019

mcollina left a comment

jasnell Oct 17, 2019

exx8 Oct 17, 2019

nodejs-github-bot commented Oct 17, 2019

exx8 commented Oct 18, 2019

nodejs-github-bot commented Nov 5, 2019

addaleax commented Nov 5, 2019

exx8 commented Nov 6, 2019 •

edited

Loading

addaleax commented Nov 6, 2019

Trott commented Jan 24, 2020

This comment has been minimized.

BridgeAR commented Jan 24, 2020

nodejs-github-bot commented Jan 24, 2020 •

edited by Trott

Loading

nodejs-github-bot commented Jan 24, 2020

Trott commented Jan 25, 2020

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

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

Conversation

exx8 commented Oct 12, 2019 • edited Loading

Checklist

exx8 commented Oct 12, 2019

exx8 commented Oct 13, 2019

bnoordhuis left a comment

Choose a reason for hiding this comment

BridgeAR Oct 14, 2019

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

exx8 commented Oct 15, 2019

BridgeAR commented Oct 15, 2019

Trott commented Oct 16, 2019

mcollina left a comment

Choose a reason for hiding this comment

jasnell Oct 17, 2019

Choose a reason for hiding this comment

exx8 Oct 17, 2019

Choose a reason for hiding this comment

nodejs-github-bot commented Oct 17, 2019

exx8 commented Oct 18, 2019

nodejs-github-bot commented Nov 5, 2019

addaleax commented Nov 5, 2019

exx8 commented Nov 6, 2019 • edited Loading

addaleax commented Nov 6, 2019

Trott commented Jan 24, 2020

This comment has been minimized.

BridgeAR commented Jan 24, 2020

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

nodejs-github-bot commented Jan 24, 2020

Trott commented Jan 25, 2020

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