Why `fs.exists` has signature `(exists)` instead of `(err, exists)` ?

510 views
Skip to first unread message

Alexey Petrushin

unread,
Aug 17, 2012, 6:57:18 PM8/17/12
to nod...@googlegroups.com
Wondering why `fs.exists` doesn't comply to async methods pattern and doesn't return error as a first argument?

It uses

fs.exists('/etc/passwd', function (exists) {
  util.debug(exists ? "it's there" : "no passwd!");
});

Instead of

fs.exists('/etc/passwd', function (err, exists) {
  util.debug(exists ? "it's there" : "no passwd!");
});

Joshua Holbrook

unread,
Aug 17, 2012, 7:00:13 PM8/17/12
to nod...@googlegroups.com
Look at the sauce for fs.exists. I think you'll find it enlightening.

(tl;dr: use fs.stat instead.)

--Josh
> --
> Job Board: http://jobs.nodejs.org/
> Posting guidelines:
> https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
> You received this message because you are subscribed to the Google
> Groups "nodejs" group.
> To post to this group, send email to nod...@googlegroups.com
> To unsubscribe from this group, send email to
> nodejs+un...@googlegroups.com
> For more options, visit this group at
> http://groups.google.com/group/nodejs?hl=en?hl=en



--
Joshua Holbrook
Head of Support
Nodejitsu Inc.
jo...@nodejitsu.com

Alexey Petrushin

unread,
Aug 17, 2012, 7:19:34 PM8/17/12
to nod...@googlegroups.com
Looking at source doesn't enlightened me. I don't see why unimportant internal implementation details should affect public interface. Especially if it's makes API inconsistent.

Bert Belder

unread,
Aug 17, 2012, 8:10:55 PM8/17/12
to nod...@googlegroups.com


On Saturday, August 18, 2012 12:57:18 AM UTC+2, Alexey Petrushin wrote:
Wondering why `fs.exists` doesn't comply to async methods pattern and doesn't return error as a first argument?

Nobody knows. It was a mistake. And now we're married.

- Bert

Jimb Esser

unread,
Aug 17, 2012, 9:49:14 PM8/17/12
to nod...@googlegroups.com
In actuality, fs.exists *only* has an err argument... or really a "not err" argument.  "fs.exists" is just "not fs.stat".  If you want fs.exists to have an error as the first argument, do "my_fs_exists = fs.stat", and it'll have exactly the same results (though will be slightly faster).

But, yeah, slightly awkwardly different signature, though the "err" argument of an "exists" query is basically meaningless, and "exists" was not originally part of the "fs" module, so it's understandable.

Alexey Petrushin

unread,
Aug 18, 2012, 12:52:47 AM8/18/12
to nod...@googlegroups.com
Hmmm, if it's implemented in such a way in order to be backward compatible - can it be changed to something like:

   if (callback.length == 1) {callback(exists)} else {callback(null, exists)}

It will support almost all cases that use older version.

dhruvbird

unread,
Aug 18, 2012, 2:27:05 AM8/18/12
to nod...@googlegroups.com


On Friday, August 17, 2012 6:49:14 PM UTC-7, Jimb Esser wrote:
In actuality, fs.exists *only* has an err argument... or really a "not err" argument.  "fs.exists" is just "not fs.stat".  If you want fs.exists to have an error as the first argument, do "my_fs_exists = fs.stat", and it'll have exactly the same results (though will be slightly faster).

But, yeah, slightly awkwardly different signature, though the "err" argument of an "exists" query is basically meaningless, and "exists" was not originally part of the "fs" module, so it's understandable.

I don't think it's meaningless.
If the fs returns an I/O error, you can't say that the file doesn't exist.


Robert Gould

unread,
Aug 19, 2012, 6:01:54 AM8/19/12
to nod...@googlegroups.com
Node has broken backwards compatibilty before and will in the futre, why not just deprecate fs.exists and remove it in 0.9? As THE black sheep in node.js it does more hurt by being there, then not being there. Not needed and not much use beyond being an insider joke for people that have been around from the start

Bruno Jouhier

unread,
Aug 19, 2012, 7:04:51 AM8/19/12
to nod...@googlegroups.com
+1.

It already got moved from "path" to "fs" and people did adjust to the change. Why not shake it one more time and fix it (or remove it)?

Nathan Rajlich

unread,
Aug 19, 2012, 1:29:58 PM8/19/12
to nod...@googlegroups.com
I'd be more inclined to just remove it. Use fs.stat() instead, or even
better just *do* whatever it is with the file that you're trying to
do. Checking for file "existence" is an anti-pattern.

Isaac Schlueter

unread,
Aug 19, 2012, 2:02:31 PM8/19/12
to nod...@googlegroups.com
We are not going to remove fs.exists.

Mikeal Rogers

unread,
Aug 19, 2012, 2:07:19 PM8/19/12
to nod...@googlegroups.com
Changing it's signature without renaming or moving it is worse than removing. It would break more code and in a way that is harder to detect.

It has a funky signature, which means you can't use most flow control libraries with it, but we can avoid issues by simply pointing people at fs.stat. Maybe a note in the docs tell people that fs.stat is a better choice?

-Mikeal

Nuno Job

unread,
Aug 19, 2012, 2:23:53 PM8/19/12
to nod...@googlegroups.com
>> Maybe a note in the docs tell people that fs.stat is a better choice?

Good idea Mikeal. Deprecation console.error && pointing people to
fs.stat in the docs should do the trick.

Bert Belder

unread,
Aug 19, 2012, 5:02:23 PM8/19/12
to nod...@googlegroups.com

I don't see enough compelling reasons to deprecate it. I think a doc addition that warns people about the funky signature and the anti-pattern would suffice.

- Bert
 

Tim Caswell

unread,
Aug 20, 2012, 11:53:54 AM8/20/12
to nod...@googlegroups.com
How about removing it from the docs and making it non-enumerable in
the fs module. Then any new developers won't know it's there unless
they are reading someone else's code. Or maybe in the docs simply say
that it shouldn't be used and is only left there so as to not break
old code. Also, how is this different from deprecation?

Arnout Kazemier

unread,
Aug 20, 2012, 11:59:19 AM8/20/12
to nod...@googlegroups.com
I really dont get why people want to depricate functions just because they dont agree with the api signature.

This is a useful function, it doesnt hurt anyone if we keep it, but it does hurt when its removed.

Dan Milon

unread,
Aug 20, 2012, 6:37:36 PM8/20/12
to nod...@googlegroups.com
Thats what PHP thought about deprecation also. See where this got them.

Mikeal Rogers

unread,
Aug 20, 2012, 7:05:30 PM8/20/12
to nod...@googlegroups.com
Because we spend a lot of time convincing library authors not to defect from the standard callback signature. Diverging from it in core sends the wrong message.

Stewart Mckinney

unread,
Aug 20, 2012, 7:11:28 PM8/20/12
to nod...@googlegroups.com
Yeah, I couldn't agree with Dan more. Consistency is super important, especially for a core lib. I mean, it could be worse, b ut, it should be addressed rather than ignored.

Dominic Tarr

unread,
Aug 22, 2012, 6:32:59 AM8/22/12
to nod...@googlegroups.com
I agree with Tim Caswell. Lets face it, that exists exists is embarassing.
removing it from the docs (replace with a link to stat) but leaving it
in the code (with a big comment) is the right compromise. once
everyone has forgotten about it, it will be easier to remove it.

I understand isaac's position. he doesn't want to remove exists,
because although it's a breaking change, and although we LOVE breaking
changes in node, what we really love about breaking changes is when a
breaking change is a breaking improvement.

but this is only a cosmetic improvement. there will be no associated
performance improvement...

Alan Gutierrez

unread,
Aug 22, 2012, 10:25:57 AM8/22/12
to nod...@googlegroups.com
On Wed, Aug 22, 2012 at 12:32:59PM +0200, Dominic Tarr wrote:
> I agree with Tim Caswell. Lets face it, that exists exists is embarassing.
> removing it from the docs (replace with a link to stat) but leaving it
> in the code (with a big comment) is the right compromise. once
> everyone has forgotten about it, it will be easier to remove it.
>
> I understand isaac's position. he doesn't want to remove exists,
> because although it's a breaking change, and although we LOVE breaking
> changes in node, what we really love about breaking changes is when a
> breaking change is a breaking improvement.
>
> but this is only a cosmetic improvement. there will be no associated
> performance improvement...

Consistency is a hobgoblin and all that. I'm glad that node doesn't waste time
on the sort of refactoring that plagued open source a few years back; endlessly
refactoring for the sake of a taxonomy that is closer to the Platonic form.

I really appreciate the way decisions to alter node are made.

However, that `exists` has a different signature from everything else has always
felt like a burr. I'm surprised it lives on. I'm surprised there is a function
that's been left in node that whenever anyone uses it, they are told not to,
because checking for existence is an anti-pattern, and `stat` is better.

That is a waste of community energy: having a bright shinny function that draws
newbies like moths, only to have the community zap them when they use it.

It it a test?

--
Alan Gutierrez - http://twitter.com/bigeasy

Alexey Petrushin

unread,
Aug 22, 2012, 1:54:15 PM8/22/12
to nod...@googlegroups.com
By the way, why "checking for existence is 'anti-pattern'"? Can You please give a link to the detailed description of such case?

Tim Caswell

unread,
Aug 22, 2012, 2:36:29 PM8/22/12
to nod...@googlegroups.com
Because if you're going to do anything with the file after checking
for it, why not just try to do the thing in the first place. It's a
race condition anyway. The file might disappear between checking for
it and trying to use it. At least with fs.stat you get the
information about the file if it exists, and a detailed error object
if it doesn't. Performance wise, this is the same number of syscalls
and less JS function calls. (fs.exists is just a wrapper around
fs.stat anyway) Also fs.exists lies sometimes. Since it can only
return a true or false, what should it do if the file exists, but
you're not able to read it? What if you're not able to enter it's
parent directory?

Alexey Petrushin

unread,
Aug 22, 2012, 4:13:59 PM8/22/12
to nod...@googlegroups.com
Thanks for explanation Tim, I got it, although, I'm not sure that it's worth it.

In most cases (except rare performance bottleneck) I'd prefer clear code like `create file unless file exists` instead of implicit logic relying on error handling like `create file rescue error`, but, this is only my opinion, not insisting on it.

Jimb Esser

unread,
Aug 22, 2012, 4:29:57 PM8/22/12
to nod...@googlegroups.com
Yeah, there are a lot of cases where "does not exists" as an "error" needs to be treated differently.  I tried making a "read from this file, return a default value if it does not exist" function without using fs.exists, and was unable to do so efficiently without relying on undocumented error code values [1].  It's very unclear if that code will work on different versions of node let alone different operating systems.  It's vitally important in cases like "rename this file to a backup before overwriting" that an "error" in the process is treated differently than "file does not exists", so fs.exists does serve a purpose, even if its signature could have been designed in a more conformant manner.

Nathan Rajlich

unread,
Aug 22, 2012, 5:10:42 PM8/22/12
to nod...@googlegroups.com
Checking "err.code" for "ENOENT" is the most cross-platform and
backwards compatible way to check for the existence of the file.

Isaac Schlueter

unread,
Aug 22, 2012, 5:32:43 PM8/22/12
to nod...@googlegroups.com
Yeah, Jimb, I'm not sure I know what you mean by "undocumented error
code values". Node does assume some familiarity with Posix error code
values, but that's a very long and well-documented tradition.

Actually, your gist is a perfect example of why fs.exists is a)
unnecessary, and b) almost always the wrong idea. In the first case,
if the file exists, but is not readable, then the fs.exists() will
return false, and your program will act as if the file is *missing*,
which is incorrect.

Jimb Esser

unread,
Aug 22, 2012, 6:27:53 PM8/22/12
to nod...@googlegroups.com, i...@izs.me
I can find no reference in the Node documentation to "ENOENT" other than in regards to DNS functions, no description at all in the File System documentation on what the error objects are (other than examples using them as booleans or opaque types to be displayed or thrown), nothing even saying an "Error" type has a "code" member you can reliably do anything with.  I would not call that "well-documented".  Coming from Windows-land, having never seen "ENOENT" before, I just assumed that was an internal libuv identifier or something.

At least in any case I tested, if a file exists and is not readable, fs.exists returns "true", as I'd expect.  There are probably some cases (perhaps if the file is in a location you do not have access to, as opposed to the fairly common case of just being unable to access a file) where the current fs.exists would return "false", but that's just a really good argument that the current fs.exists should only return false if stat returned an error *and* that error was "ENOENT" or whichever set of error codes indicates non-existence, not that the API is flawed.  Abstracting away exactly which behaviors or error codes indicate "existence" is useful.

Dan Milon

unread,
Aug 22, 2012, 6:34:14 PM8/22/12
to nod...@googlegroups.com
Isaac, i am curious why you believe fs.exists should stay as is.
I found your previous mail kind of "autarchic". Some
communication/reasoning will surely make each side understand better the
pros/cons.

danmilon.

Scott González

unread,
Aug 22, 2012, 6:45:03 PM8/22/12
to nod...@googlegroups.com
On Wed, Aug 22, 2012 at 6:34 PM, Dan Milon <danm...@gmail.com> wrote:
Isaac, i am curious why you believe fs.exists should stay as is.

Stewart Mckinney

unread,
Aug 22, 2012, 11:30:30 PM8/22/12
to nod...@googlegroups.com
Why not just decorate fs.exists with arguments.caller.length and call it a day? It seems like there are arguments on both sides here to accept the new standard and the old tradition. Eventually phasing out the old tradition has to happen ( especially since you know, not 1.0 or anything here ), but i'm just curious if there's any resistance to that interim solution right now. Seems like that could work, and polymorphism is not necessarily a forsaken tradition in programming.

Also,


Why does GMail think that polymorphism is not a word? Seriously?

--

Matt

unread,
Aug 22, 2012, 11:32:16 PM8/22/12
to nod...@googlegroups.com
In javascript you can't know how many arguments a callback takes. Only how many arguments were passed to a function.

Stewart Mckinney

unread,
Aug 22, 2012, 11:36:59 PM8/22/12
to nod...@googlegroups.com
Ah, nevermind. Realized why. :/

On Wed, Aug 22, 2012 at 11:30 PM, Stewart Mckinney <lord...@gmail.com> wrote:

Nathan Rajlich

unread,
Aug 22, 2012, 11:58:31 PM8/22/12
to nod...@googlegroups.com
callback.length

Nathan Rajlich

unread,
Aug 22, 2012, 11:59:42 PM8/22/12
to nod...@googlegroups.com
Also, somebody really should turn this thread into a pull request ;)

Mikeal Rogers

unread,
Aug 23, 2012, 12:49:31 AM8/23/12
to nod...@googlegroups.com
This suggested gained a lot of +1's so i want to talk about it a moment.

I don't think it's suitable to *remove* APIs that still exist from the docs or hide them from being enumerable.

There are just too many node programs out there now and people who want to understand existing code need to read the documentation for an API that *is still active* should be able to find and view it and to debug that API via test code in the repl which making non-enumerable makes annoying.

This just isn't the grown up way to handle a mistake or a deprecation.

We need to call out in the docs that 1) this exists and is still available 2) it should not be used 3) why it should not be used and links to doing things the "right" way. Pretending it doesn't exist when it still does is like pretending we never made this mistake even though it still works in node. Let's take responsibility for this and do what's best for all of the existing and future users of node.

-Mikeal

Isaac Schlueter

unread,
Aug 23, 2012, 11:12:30 AM8/23/12
to nod...@googlegroups.com
Scott Gonzalez posted a good link about deprecation. I copied it to a
wiki page. This is the official policy:
https://github.com/joyent/node/wiki/deprecation

I'd accept a pull request that explains the oddness of fs.exists in
the documentation. Making it non-enumerable or removing it from the
docs is a bad idea, on further consideration.

I'd also accept a pull request that documents all the Posix error
codes, though really, that should probably be part of the libuv
documentation. (But writing docs for libuv is a much larger pull
request.)

rektide

unread,
Aug 23, 2012, 4:34:24 PM8/23/12
to nod...@googlegroups.com
I would really really appreciate it if Node were to consistently get behind err being the first param, even when it's nonsensical.

Readline.question could stand being made from answer => void to err,answer => void as well.

On Friday, August 17, 2012 6:57:18 PM UTC-4, Alexey Petrushin wrote:
Wondering why `fs.exists` doesn't comply to async methods pattern and doesn't return error as a first argument?

It uses

fs.exists('/etc/passwd', function (exists) {
  util.debug(exists ? "it's there" : "no passwd!");
});

Instead of

fs.exists('/etc/passwd', function (err, exists) {
  util.debug(exists ? "it's there" : "no passwd!");
});

Bruno Jouhier

unread,
Aug 23, 2012, 4:45:42 PM8/23/12
to nod...@googlegroups.com, i...@izs.me
fs: stability 3 (Stable) => Backwards compatibility is guaranteed.

There was an opportunity to fix it when moving it from path to fs. But now it's too late!

Well, that's just life!

mscdex

unread,
Aug 23, 2012, 6:23:20 PM8/23/12
to nodejs
On Aug 23, 4:45 pm, Bruno Jouhier <bjouh...@gmail.com> wrote:
> fs: stability 3 (Stable) => Backwards compatibility is guaranteed.
>
> There was an opportunity to fix it when moving it from path to fs. But now
> it's too late!
>
> Well, that's just life!

As previously suggested, we could still add a note to fs.exists'
description to point people to fs.stat and let them know that
fs.exists shouldn't be used (and why) or at least that there can be
gotchas.
Reply all
Reply to author
Forward
0 new messages