-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
doc: add example for file existence with fs.stat #8585
Conversation
Add an example on how to test if a file exists with fs.stat. Also add a link to the Common System Errors. Fixes: https://github.com/nodejs/issues/6752
If there is need to document an example of
More collaborator feedback needed here. |
If you just want to check that a file exists, If you want to manipulate the file, stat-then-open (or stat-then-move, etc.) is subject to TOCTOU race conditions. The right way is to simply open the file and handle the error if it doesn't exist or isn't accessible. |
Your effort is much appreciated @connium, but I'm -1 on landing this. What I'd like to see added is the part where |
@connium .. the clarification on the error code is definite +1. @imyller @bnoordhuis ... I think I'd be fine with adding this example so long as there's also a note added saying exactly what @bnoordhuis is describing in his comment... that while this example works, there are better and more reliable ways of accomplishing it. I would definitely sign off on it with that added. |
I agree with @jasnell and I won't object to this PR being landed, if doc is amended with valid points @bnoordhuis made in his comment. |
@imyller @bnoordhuis @jasnell thanks for your feedback! I wasn't aware of the complexity of doing file existence checks the right way 😃 |
I think that the doc is much better now. This adds important |
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.
LGTM
Instead, user code should open/read/write the file directly and handle the | ||
error raised if the file is not available. | ||
|
||
To check if a file exists without manipulating it afterwards, [`fs.access()`] is recommended. |
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: long line here
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.
Sorry, I missed that. It is fixed now.
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.
LGTM with a nit.
Add an example on how to test if a file exists with fs.stat. Also add a link to the Common System Errors. Fixes: https://github.com/nodejs/issues/6752 PR-URL: #8585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Landed in 23b9625. Thank you! |
Add an example on how to test if a file exists with fs.stat. Also add a link to the Common System Errors. Fixes: https://github.com/nodejs/issues/6752 PR-URL: #8585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Add an example on how to test if a file exists with fs.stat. Also add a link to the Common System Errors. Fixes: https://github.com/nodejs/issues/6752 PR-URL: #8585 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Checklist
Affected core subsystem(s)
doc
Description of change
Add an example on how to test if a file exists with fs.stat. Also add
a link to the Common System Errors.
Fixed with help of @dschnei @narf101010
Fixes: #6752