-
Notifications
You must be signed in to change notification settings - Fork 30k
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
fs: improve fs.watch ENOSPC error message #21846
Conversation
This comment has been minimized.
This comment has been minimized.
179598d
to
b2b052c
Compare
The test probably needs some per-platform tweaking, so: |
b2b052c
to
fd21126
Compare
} | ||
|
||
setTimeout(() => { | ||
if (!finished && processes.length < 200) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
6f4bd38
to
b156539
Compare
CI: https://ci.nodejs.org/job/node-test-pull-request/16123/ Might be good to get another @nodejs/tsc review – this error comes with a code already, so I don’t think this has to be semver-major. |
let accumulated = ''; | ||
gatherStderr.on('data', common.mustCallAtLeast((chunk) => { | ||
accumulated += chunk; | ||
// console.log(chunk); |
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.
TBD (to be deleted)
lib/internal/fs/watchers.js
Outdated
@@ -161,7 +162,9 @@ FSWatcher.prototype.start = function(filename, | |||
const error = errors.uvException({ | |||
errno: err, | |||
syscall: 'watch', | |||
path: filename | |||
path: filename, | |||
description: err === UV_ENOSPC ? |
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.
Nit: Might want to call it message?
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.
+1 to call it message
. By the way if the key is message
it currently will not appear in the final error object as an additional property whereas description
will.
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.
ping @addaleax
Ping again @addaleax |
Providing `No space left on device` is misleading in this case. Replace it with something that describes it more accurately. Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959
b156539
to
310fdd4
Compare
Rebased, with comments addressed. |
Landed in 13245dc |
Providing `No space left on device` is misleading in this case. Replace it with something that describes it more accurately. Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959 PR-URL: #21846 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Providing `No space left on device` is misleading in this case. Replace it with something that describes it more accurately. Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959 PR-URL: #21846 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Providing
No space left on device
is misleading in this case.Replace it with something that describes it more accurately.
Refs: https://stackoverflow.com/questions/22475849/node-js-error-enospc/32600959
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes