This repository has been archived by the owner. It is now read-only.

fs.exists does not follow node conventions #8369

Closed
fresheneesz opened this Issue Sep 14, 2014 · 11 comments

Comments

Projects
None yet
8 participants
@fresheneesz

fresheneesz commented Sep 14, 2014

fs.exists is the only function in all of node that has a callback that isn't in the errback form (a callback that takes an error as its first parameter). This wastes about 10 minutes of my time every time i forget about it. Please change this to be consistent with the rest of the node ecosystem.

fs.exists('/some/path', function(err, exists) {
   if(err) throw err
   if(exists) //do whatever
})
@micnic

This comment has been minimized.

micnic commented Sep 15, 2014

If you look in the Node's docs you'll see that fs.exists is kept for historical reasons and it is not recommended to use it because it may lead to race conditions when another process may remove the file between the calls to fs.exists() and fs.open(). From the other hand, there may be no error while checking if a file exists or not, there is just the existence status, that's all, the error parameter is redundant in this callback.

@fresheneesz

This comment has been minimized.

fresheneesz commented Sep 15, 2014

Hmm, while I agree that the error parameter would be redundant, i don't agree that being the only special case is worth removing that redundancy. In any case, it sounds like its a moot point if exists is essentially deprecated. Tho I'm not sure i'm comfortable using exceptions that way..

@vkurchatkin

This comment has been minimized.

Member

vkurchatkin commented Sep 15, 2014

is also causes problems with promisify utilities, as they expect error first callback

@indutny

This comment has been minimized.

Member

indutny commented Sep 15, 2014

@amir-s

This comment has been minimized.

amir-s commented Sep 18, 2014

How about changing it in the way that it gives the callback two parameters if the provided callback wants two. This way we have the backward compatibility and the consistency of callback definitions in Node.

@trevnorris

This comment has been minimized.

trevnorris commented Sep 20, 2014

+1 from me for changing the API. There's not really a way to deprecate the change, so I say we standardize.

@indutny any objections from you?

cjihrig added a commit to cjihrig/node that referenced this issue Sep 21, 2014

fs: add errback style support to exists()
fs.exists() does not currently support Node's standard
callback signature. This commit adds errback support while
maintaining backwards compatibility. Closes nodejs#8369.
@cjihrig

This comment has been minimized.

cjihrig commented Sep 21, 2014

If you decide to make this change, I've opened a PR. If you decide against it, feel free to close it.

@refack

This comment has been minimized.

Member

refack commented Sep 21, 2014

If we're digging into this can I ask for an existsSync or statSync that doesn't throw on UV_EEXIST, so that requiring modules will become exception free (currently using debugger's break on exceptions is useless).
I have a piece of code that implements it (as a special method to be used by module.js for now).

@trevnorris

This comment has been minimized.

trevnorris commented Sep 22, 2014

@cjihrig Thanks for the PR. I'll continue further discussion there.

@refack I assume UV_EEXIST happens because the command that's attempting to be executed does not exist. Right now throwing in such a case is "standard". If it were to be changed then I believe it should happen across the board (e.g. there's a bigger discussion to be had about this).

@refack

This comment has been minimized.

Member

refack commented Sep 23, 2014

@trevnorris I'm talking about a special case optimization - 9dae54a
I don't think this needs to be across the board.

@trevnorris

This comment has been minimized.

trevnorris commented Sep 29, 2014

This is being overridden by #8418.

@trevnorris trevnorris closed this Sep 29, 2014

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.