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

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

Closed
wants to merge 15 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
fa5984b
fs: watch signals for recursive incompatibility
Oct 12, 2019
b731dac
Some lints to the fs.js file.
Oct 12, 2019
20d6e4c
A new error ERR_OPTION_INCOMPATIBLE_WITH_PLATFORM is added to node.
Oct 12, 2019
98e7561
update the relevant test: if on win or osx no exception should be thr…
Oct 12, 2019
d985a42
Documenting the new changes:
Oct 12, 2019
bdb2a7d
A style change
exx8 Oct 12, 2019
c6373f3
fix according to review
Oct 13, 2019
194ca4d
fix to documentation
Oct 13, 2019
a0b5dd5
Update doc/api/fs.md
exx8 Oct 14, 2019
b0fc0eb
Update doc/api/fs.md
exx8 Oct 14, 2019
cb24297
Update lib/internal/errors.js
exx8 Oct 14, 2019
7e23cef
reformatting the error definition, and refactor the test
Oct 15, 2019
120a471
updating test.
Oct 15, 2019
632c0e1
fixup! updating test.
Trott Jan 24, 2020
b3db344
fixup! fixup! updating test.
Trott Jan 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions doc/api/errors.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2412,6 +2412,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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Trott marked this conversation as resolved.
Show resolved Hide resolved

Used when a feature that is not available
to the current platform which is running Node.js is used.

<a id="ERR_STREAM_HAS_STRINGDECODER"></a>
#### `ERR_STREAM_HAS_STRINGDECODER`

Expand Down
2 changes: 2 additions & 0 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3766,6 +3766,8 @@ The `fs.watch` API is not 100% consistent across platforms, and is
unavailable in some situations.

The recursive option is only supported on macOS and Windows.
An `ERR_FEATURE_UNAVAILABLE_ON_PLATFORM` exception will be thrown
when the option is used on a platform that does not support it.

#### Availability

Expand Down
7 changes: 5 additions & 2 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,8 @@ const {
ERR_FS_FILE_TOO_LARGE,
ERR_INVALID_ARG_VALUE,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_CALLBACK
ERR_INVALID_CALLBACK,
ERR_FEATURE_UNAVAILABLE_ON_PLATFORM
},
uvException
} = require('internal/errors');
Expand Down Expand Up @@ -129,6 +130,7 @@ let FileReadStream;
let FileWriteStream;

const isWindows = process.platform === 'win32';
const isOSX = process.platform === 'darwin';


function showTruncateDeprecation() {
Expand Down Expand Up @@ -1359,7 +1361,8 @@ function watch(filename, options, listener) {

if (options.persistent === undefined) options.persistent = true;
if (options.recursive === undefined) options.recursive = false;

if (options.recursive && !(isOSX || isWindows))
throw new ERR_FEATURE_UNAVAILABLE_ON_PLATFORM('watch recursively');
if (!watchers)
watchers = require('internal/fs/watchers');
const watcher = new watchers.FSWatcher();
Expand Down
4 changes: 4 additions & 0 deletions lib/internal/errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,10 @@ E('ERR_FALSY_VALUE_REJECTION', function(reason) {
this.reason = reason;
return 'Promise was rejected with falsy value';
}, Error);
E('ERR_FEATURE_UNAVAILABLE_ON_PLATFORM',
'The feature %s is unavailable on the current platform' +
', which is being used to run Node.js',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the message could actually include the used platform. That way it would be easier to identify the used platform in case the logs would e.g., be transferred to a third party application that monitors the errors.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm sorry, I currently don't have time for continue working on this PR. If someone else would like to make this change I'll appreciate it. thanks.

TypeError);
E('ERR_FS_FILE_TOO_LARGE', 'File size (%s) is greater than 2 GB', RangeError);
E('ERR_FS_INVALID_SYMLINK_TYPE',
'Symlink type must be one of "dir", "file", or "junction". Received "%s"',
Expand Down
7 changes: 5 additions & 2 deletions test/parallel/test-fs-watch-recursive.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@

const common = require('../common');

if (!(common.isOSX || common.isWindows))
common.skip('recursive option is darwin/windows specific');

const assert = require('assert');
const path = require('path');
Expand All @@ -20,6 +18,11 @@ const testsubdir = fs.mkdtempSync(testDir + path.sep);
const relativePathOne = path.join(path.basename(testsubdir), filenameOne);
const filepathOne = path.join(testsubdir, filenameOne);

if (!common.isOSX && !common.isWindows) {
common.expectsError(() => fs.watch(testDir, { recursive: true }),
{ code: 'ERR_FEATURE_UNAVAILABLE_ON_PLATFORM' });
return;
}
exx8 marked this conversation as resolved.
Show resolved Hide resolved
const watcher = fs.watch(testDir, { recursive: true });

let watcherClosed = false;
Expand Down