Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

fs: add access() and accessSync() #8714

Closed
wants to merge 1 commit into from
Closed

fs: add access() and accessSync() #8714

wants to merge 1 commit into from

Conversation

cjihrig
Copy link

@cjihrig cjihrig commented Nov 12, 2014

fs.exists() and fs.existsSync() do not follow typical node conventions. access() and accessSync() are added as alternatives in this commit.

@trevnorris I opened this as a separate PR because it was decided in #8418 that exists() and existsSync() would not be deprecated yet. One thought that I had is that these functions might seem less C-like if they returned true on success instead of the 0 returned by libuv. Thoughts?

@@ -656,10 +656,46 @@ that leaves you vulnerable to race conditions: another process may remove the
file between the calls to `fs.exists()` and `fs.open()`. Just open the file
and handle the error when it's not there.

`fs.exists()` will be deprecated in the future in favor of `fs.access()`.

Choose a reason for hiding this comment

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

I would just say "fs.exists() will be deprecated." since we don't recommend using anything to check for a file exists then blindly reading from it (which is how fs.exists() is being used).

@trevnorris
Copy link

Awesome stuff. Thanks for getting this in all the way from libuv to here.

Just a few comments, but LGTM.

@trevnorris
Copy link

@cjihrig

One thought that I had is that these functions might seem less C-like if they returned true on success instead of the 0 returned by libuv.

Good point. There are a couple different ways this could work. Honestly there wouldn't have to be a return value at all, since if everything is fine then no error object will exist. e.g.

fs.access('/some/thing', function(er) {
  if (!er) {
    // The access check must have succeeded.
  }
});

And for fs.accessSync() the user doesn't even need to check the return value since it'll throw otherwise.

Though this type of API change might be drastic. I definitely like that it'll return a useful error (e.g. ENOENT or ENOTDIR). For now just change it to true/false.

@cjihrig
Copy link
Author

cjihrig commented Nov 17, 2014

@trevnorris updated based on your comments.

cjihrig added a commit to nodejs/node that referenced this pull request Dec 15, 2014
fs.exists() and fs.existsSync() do not follow the typical
error first callback convention. access() and accessSync()
are added as alternatives in this commit.

Fixes: nodejs/node-v0.x-archive#8714
PR-URL: #114
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Domenic Denicola <domenic@domenicdenicola.com>
fs.exists() and fs.existsSync() do not follow the typical
error first callback convention. access() and accessSync()
are added as alternatives in this commit.
@timoxley
Copy link

bump.

@trevnorris
Copy link

Thanks for the bump. Landed in 2944934.

@trevnorris trevnorris closed this Jan 13, 2015
cjihrig added a commit that referenced this pull request Jan 13, 2015
fs.exists() and fs.existsSync() do not follow the typical error first
callback convention. access() and accessSync() are added as alternatives
in this commit.

PR-URL: #8714
Reviewed-by: Trevor Norris <trev.norris@gmail.com>
steelbrain referenced this pull request in paulmillr/chokidar Mar 30, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants