Skip to content
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

gripe: deprecating fs.exists/existsSync #1592

Closed
Amorelandra opened this issue May 2, 2015 · 207 comments
Closed

gripe: deprecating fs.exists/existsSync #1592

Amorelandra opened this issue May 2, 2015 · 207 comments

Comments

@Amorelandra
Copy link
Member

@Amorelandra Amorelandra commented May 2, 2015

After noticing in the node docs that fs.exists and fs.existsSync were going to be deprecated, I took at look at the iojs docs to see that it had in fact been.

This is really annoying and seems like it's based out of the assumption that every developer ever is only checking the existence of a file prior to reading/writing in some async context. While I understand the sentiment behind wanting to avoid 'unnecessary abstractions' or race conditions, this is unnecessary.

Whereas I was previously able to handle the checking of a file's existence with a single boolean variable, I'm now given no other option but to either try/catch or make my code async and listen for error events.

This feels like prescriptivism, as I can't think of a single reason why a stern warning of the potential implications and/or examples of caveats to its use wouldn't have sufficed.

Can anyone help me understand why this was necessary (beyond pushing the all-async-all-the-time paradigm (that doesn't always necessarily apply (particularly in the case of synchronous CLI tooling)))?

Or perhaps can I just submit a PR that un-deprecates this perfectly good functionality?

EDIT: I am happy to provide any additional documentation that is deemed necessary.

@mscdex mscdex added the question label May 2, 2015
@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented May 2, 2015

This is really annoying and seems like it's based out of the assumption that every developer ever is only checking the existence of a file prior to reading/writing in some async context.

That's not quite how it is.

See fs.exists() in the docs .. it's been replaced by fs.access()

See #114 for more reference.

@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented May 2, 2015

Reason being: fs.exists() used an API format that was unlike anything else in our API, and could produce results that were unexpected to those who didn't otherwise know.

@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

fs.access doesn't seem like a replacement given that it also throws when there are any accessibility checks that fail. I don't want/need to do error handling every time I check for a file's existence. I don't think it's fair to call this a replacement.

@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented May 2, 2015

I'm now given no other option but to either try/catch or make my code async and listen for error events.

That would be correct.

@Fishrock123 Fishrock123 closed this May 2, 2015
@Fishrock123 Fishrock123 reopened this May 2, 2015
@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

OK, close the issue before I can even reply? Awesome!

@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

Deprecation with this kind of logic/motivation is only going to serve to make this platform less hospitable to people doing things outside of what features core is focused on.

I still don't see any reason why thorough documentation isn't an adequate response to any of the concerns that have been mentioned thus far.

@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

I'm also pretty concerned at the realization that this is how we're treating issues created around valid questions/concerns.

Slapping down an open issue within 20 minutes of its creation is pretty hostile.

@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented May 2, 2015

Original discussion can be found at nodejs/node-v0.x-archive#8418 & #103

@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented May 2, 2015

Slapping down an open issue within 20 minutes of its creation is pretty hostile.

The GitHub close issue button is in the same location a cancel button usually is. This isn't uncommon for people to occasionally do. Is the intent not clear since I directly re-opened it? :/

@mscdex

This comment has been minimized.

Copy link
Contributor

@mscdex mscdex commented May 2, 2015

If you're concerned about having to do a try-catch everywhere for fs.accessSync(), why not just make your own wrapper function(s)?:

var fs = require('fs');
function existsSync(filename) {
  try {
    fs.accessSync(filename);
    return true;
  } catch(ex) {
    return false;
  }
}
function exists(filename, cb) {
  fs.access(filename, cb);
  // or if want exactly the same old API:
 //fs.access(function(err) { cb(err ? false : true); });
}
@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

or why not leave the wrapper the way it is and let me continue to use fs.existsSync? 😸

@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented May 2, 2015

perfectly good functionality?

or why not leave the wrapper the way it is and let me continue to use fs.existsSync?

That same thinking can be applied to just adding more and more sugar to core too. There is plenty discussion in the issues I liked above. At minimum, it wasn't particularly perfect, but it wasn't broken either.

@mscdex

This comment has been minimized.

Copy link
Contributor

@mscdex mscdex commented May 2, 2015

As previously mentioned, the majority use case for fs.exists()/fs.existsSync() was to check if a file existed before opening a file. Removing it and recommending people just use fs.open()/fs.openSync() solves the problems associated with that majority use case.

@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

@Fishrock123 let's discuss how we can make it good enough to leave in.

@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

Should I create a new issue, or can we discuss the things that need to be improved on this one?

@bnoordhuis

This comment has been minimized.

Copy link
Member

@bnoordhuis bnoordhuis commented May 2, 2015

This has been discussed to death already. Moving to close.

@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

@bnoordhuis How about if you're tired of discussing this particular issue: don't involve yourself.

@bnoordhuis

This comment has been minimized.

Copy link
Member

@bnoordhuis bnoordhuis commented May 2, 2015

Kind of hard to do when I get 15 new emails in the 30 minutes I'm away from my computer... at any rate, my point is that you're unlikely to bring up anything that hasn't been brought up before.

@Fishrock123 Fishrock123 added the fs label May 2, 2015
@Amorelandra

This comment has been minimized.

Copy link
Member Author

@Amorelandra Amorelandra commented May 2, 2015

@bnoordhuis understandable. Apologies for contributing to your inbox spam.

I'll take a look at the source, read up on previous conversations, and see what I can do to help fix this.

@sindresorhus

This comment has been minimized.

Copy link

@sindresorhus sindresorhus commented May 2, 2015

I made a module for this as there's still genuine use-cases for checking if a file exists without reading the file afterwards.

@ChALkeR

This comment has been minimized.

Copy link
Member

@ChALkeR ChALkeR commented May 3, 2015

Btw, what's wrong with fs.access? fs.exists has inconvenient api, for example: petkaantonov/bluebird#418
fs.access in a form fs.access(path, callback) looks to do exactly what fs.exists should have done from the start.
Or am I missing something?

@imyller

This comment has been minimized.

Copy link
Member

@imyller imyller commented May 4, 2015

Just stopping by to say that I agree with the point @emilyrose makes here.

It is true that fs.access can fully replace fs.exists and fs.existSync functionality. Especially with wrappers like @sindresorhus kindly implemented. Still many newcomers to the platform will be left looking for simple way to get boolean true/false for file existence as fs.access might not be obvious to them.

I personally think that it is, in some cases, acceptable for io.js core APIs to offer functionality that emphasizes simplicity and rapid-development over formal conventions. fs.exists might belong to that category.

@benjamingr

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr commented May 4, 2015

@emilyrose the reason it was deprecated is because fs.exists has a broken API - I opened the deprecation issue to either remove or fix this. fs.exists typically also suffers from race condition but you can use fs.access to easily do what you did with exists before or heck - use a userland package there are dozens on NPM.

@benjamingr

This comment has been minimized.

Copy link
Member

@benjamingr benjamingr commented May 4, 2015

@imyller that sounds like a documentation issue - and a good pull request

@imyller

This comment has been minimized.

Copy link
Member

@imyller imyller commented May 4, 2015

@benjamingr that is true. I do agree that fs.access has the correct API and fs.exists has issues, but I think the point of this issue is that there still are solid reasons to have simple file-exists-or-not function returning a boolean.

Additionally, one might want a documentation on why you can create full HTTP server implementation with one-liner, while needing many more lines to check if some local file exists ;)

@rlidwka

This comment has been minimized.

Copy link
Contributor

@rlidwka rlidwka commented May 4, 2015

I think the point of this issue is that there still are solid reasons to have simple file-exists-or-not function returning a boolean.

Well you can't even tell if file exists or not with the fs.exists, because in case of an error it'll fail silently even if file does exist. And it does nothing fs.stat can't do.

I don't see a single reason why such an api should exist in the core.

Additionally, one might want a documentation on why you can create full HTTP server implementation with one-liner

Don't worry, the entire http2 community is working on it. In a year or so you won't be able to create http server without getting ssl certificates, and it will surely not be a one-liner anymore

aldahick added a commit to crossroads-education/eta-lib that referenced this issue Jun 2, 2016
See nodejs/node#1592 for why this is necessary
@rotemdan

This comment has been minimized.

Copy link

@rotemdan rotemdan commented Jun 5, 2016

I have an existing file I'm opening for reading and appending, e.g.:

var fs = require("fs");

fs.open("myFile.txt", "a+", function(fd) {
    // ..
})

But I only want to allow the file to successfully open if it already exists, and throw an error otherwise.

I haven't found any way to achieve this without some form of separate existence checking. There is no flag that corresponds to this requirement:

(from the node documentation):

  • 'w' - Open file for writing. The file is created (if it does not exist) or truncated (if it exists).
  • 'wx' - Like 'w' but fails if path exists.
  • 'w+' - Open file for reading and writing. The file is created (if it does not exist) or truncated (if it exists).
  • 'wx+' - Like 'w+' but fails if path exists.
  • 'a' - Open file for appending. The file is created if it does not exist.
  • 'ax' - Like 'a' but fails if path exists.
  • 'a+' - Open file for reading and appending. The file is created if it does not exist.
  • 'ax+' - Like 'a+' but fails if path exists.

Changing to ax+ instead of a+ would cause open to error if the file exists but that's the opposite of what is needed here. Is there any way to error if it doesn't exist?

(My current workaround is to use r+ but that means I have to check the current size of the file before every write operation. This creates another problem: now every write operation may be vulnerable to a race condition by itself)

@bnoordhuis

This comment has been minimized.

Copy link
Member

@bnoordhuis bnoordhuis commented Jun 5, 2016

@rotemdan

const { O_APPEND, O_WRONLY } = require('constants');
fs.open(filename, O_APPEND | O_WRONLY, callback);
@rotemdan

This comment has been minimized.

Copy link

@rotemdan rotemdan commented Jun 5, 2016

@bnoordhuis

Thanks. I noticed these flags in the linux man page, but they don't seem to be documented officially in node.

Question: is this guaranteed to work on windows as well? and in general is it truly cross-platform? (I'm guessing the relevant libuv source code for windows is here).

@bnoordhuis

This comment has been minimized.

Copy link
Member

@bnoordhuis bnoordhuis commented Jun 5, 2016

Not all flags work on Windows (e.g. O_NOATIME) but most of them do.

It's an undocumented but pretty stable feature. The mode strings themselves are implemented in terms of O_flag constants, e.g., 'a' is shorthand for O_APPEND | O_CREAT | O_WRONLY.

@neuroscr

This comment has been minimized.

Copy link

@neuroscr neuroscr commented Jun 14, 2016

Was just about to open an issue on this. Documentation is unclear about access being a replacement. I think the real issue here is how the not found is communicated. If access/stat are to be used for an existent check, then an exception on not found is too severe. Wouldn't returning that it's not FRWX sufficient?

@dzcpy

This comment has been minimized.

Copy link

@dzcpy dzcpy commented Jun 25, 2016

fs.exists is really necessary. If the api is broken can we fix it rather than deprecate it? Or otherwise, can we provide an option for fs.access, if the second parameter mode === fs.E_OK then do not throw the error and returns the boolean?

@ChALkeR

This comment has been minimized.

Copy link
Member

@ChALkeR ChALkeR commented Jun 25, 2016

@andyhu Once again: fs.exists doesn't do what one might think it's doing judging by its name, and that is a problem.

Just use fs.access as a replacement, it does exactly the same for all fs.exists usecases (except for having the correct callback signature).

@trevnorris

This comment has been minimized.

Copy link
Contributor

@trevnorris trevnorris commented Jun 27, 2016

Seems at this point the ask is for an API that does:

function doesReallyExistSync(path) {
  try { return !fs.accessSync(path, fs.F_OK) } catch (e) { return false }
}

TBH I understand wanting such a simple API, but the problem is it's not actually that simple. Take the following example where directory b is a symlink back to a:

fs.accessSync('./a' + '/b/a'.repeat(60) + '/c', fs.F_OK)

Which will result in the following:

Error: ELOOP: too many symbolic links encountered, access './a/b/[...]/a/c
    at Error (native)
    at Object.fs.accessSync (fs.js:203:11)

Is node also supposed to add the logic of checking for ELOOP and running the path through fs.realpath() to get around this issue? If so then I'd expect node to do it's best to compensate for any platform specific issues in the API. Here things get a bit hairy, and suddenly an API at first glance was one line becomes more complex. Enough, IMO, to merit its own module (there are commonly used smaller modules out there).

@dfabulich

This comment has been minimized.

Copy link
Contributor

@dfabulich dfabulich commented Jun 28, 2016

I filed PR #7455 to improve the performance of existsSync beyond what's possible in pure JS (by avoiding throwing an ignored exception).

@dfabulich

This comment has been minimized.

Copy link
Contributor

@dfabulich dfabulich commented Jun 28, 2016

@trevnorris Your accessSync example is pretty funky! Luckily, we already know what the right thing to do with that is: existsSync returns false in that case. LGTM, so let's just undeprecate that.

@imyller

This comment has been minimized.

Copy link
Member

@imyller imyller commented Jun 28, 2016

Just for reference I looked up how other languages/runtimes do this:

Python 3 (uses stat, "if stat fails, file is assumed not to exist" -method):

# Does a path exist?
# This is false for dangling symbolic links on systems that support them.
def exists(path):
    """Test whether a path exists.  Returns False for broken symbolic links"""
    try:
        os.stat(path)
    except OSError:
        return False
    return True

Java (JDK 8) (uses stat, "if stat fails, file is assumed not to exist" -method):

JNIEXPORT jint JNICALL
Java_java_io_UnixFileSystem_getBooleanAttributes0(JNIEnv *env, jobject this,
                                                  jobject file)
{
    jint rv = 0;

    WITH_FIELD_PLATFORM_STRING(env, file, ids.path, path) {
        int mode;
        if (statMode(path, &mode)) {
            int fmt = mode & S_IFMT;
            rv = (jint) (java_io_FileSystem_BA_EXISTS
                  | ((fmt == S_IFREG) ? java_io_FileSystem_BA_REGULAR : 0)
                  | ((fmt == S_IFDIR) ? java_io_FileSystem_BA_DIRECTORY : 0));
        }
    } END_PLATFORM_STRING(env, path);
    return rv;
}

Perl (uses stat, -e (exists) op is 1/true if stat has returned any results, false otherwise)

my %op = (
    r => sub { cando($_[0], S_IRUSR, 1) },
    w => sub { cando($_[0], S_IWUSR, 1) },
    x => sub { cando($_[0], S_IXUSR, 1) },
    o => sub { $_[0][4] == $>           },

    R => sub { cando($_[0], S_IRUSR, 0) },
    W => sub { cando($_[0], S_IWUSR, 0) },
    X => sub { cando($_[0], S_IXUSR, 0) },
    O => sub { $_[0][4] == $<           },

    e => sub { 1 }     <--- exists

PHP (uses stat, "if stat fails, file is assumed not to exist" -method):
Snippet from (filestat.c):

/* {{{ proto bool file_exists(string filename)
   Returns true if filename exists */
FileFunction(PHP_FN(file_exists), FS_EXISTS)

...

    case FS_EXISTS:
        RETURN_TRUE;

Go (uses stat with additional library function to test returned error for file existence clues):

if _, err := os.Stat("./thefile.ext"); err != nil {
    if os.IsNotExist(err) {
        // file does not exist
    } else {
        // other error
    }
}

Go language approach is the most interesting: they provide standard stat and then have two separate core library convenience functions for determining if returned error indicates file existence or non-existence os.IsExists(err) and os.IsNotExists(err).

With exception of Go language, most common method for implementing "file exists" test seems to be just running stat and assuming file existence if anything / no error is returned.

@trevnorris

This comment has been minimized.

Copy link
Contributor

@trevnorris trevnorris commented Jun 28, 2016

@dfabulich

we already know what the right thing to do with that is: existsSync returns false in that case.

Not exactly. fs.realpath() should resolve the symbolic links to a path that fs.access() can handle (there's a bug in v6 where that doesn't work, for which I'm working on). So if the user got ELOOP they could then call the operation again with fs.access(fs.realpath(path));. Simply returning false would be technically incorrect.

@imyller

This comment has been minimized.

Copy link
Member

@imyller imyller commented Jul 25, 2016

For those interested:

I've published a userland module fs-exists-nodeback

https://github.com/imyller/node-fs-exists-nodeback

When loaded the module polyfills fs.exists to support both original Node.js callback style and standard error first callback style in a backward compatible way.

This may offer solution to those having issues with the callback(boolean) not working with libraries expecting functions with nodeback standard callback.

@Fishrock123

This comment has been minimized.

Copy link
Member

@Fishrock123 Fishrock123 commented Oct 5, 2016

I think 7b5ffa4 should fix / address this.

It undeprecates existsSync() and keeps the inconsistent-callback exists() deprecated.

jasnell added a commit that referenced this issue Oct 6, 2016
This has been dragged through various long discussions and has been
elevated to the CTC multiple times.

As noted in
#7455 (comment),
while this API is still generally considered an anti-pattern, there are
still use-cases it is best suited for, such as checking if a git rebase
is in progress by looking if ".git/rebase-apply/rebasing" exists.

The general consensus is to undeprecate just the sync version, given
that the async version still has the "arguments order inconsistency"
problem.

The consensus at the two last CTC meetings this came up at was also
to undeprecate existsSync() but keep exists() deprecated.
See: #8242 &
#8330

(Description write-up by @Fishrock123)

Fixes: #1592
Refs: #4217
Refs: #7455
PR-URL: #8364

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Fishrock123 added a commit that referenced this issue Oct 11, 2016
This has been dragged through various long discussions and has been
elevated to the CTC multiple times.

As noted in
#7455 (comment),
while this API is still generally considered an anti-pattern, there are
still use-cases it is best suited for, such as checking if a git rebase
is in progress by looking if ".git/rebase-apply/rebasing" exists.

The general consensus is to undeprecate just the sync version, given
that the async version still has the "arguments order inconsistency"
problem.

The consensus at the two last CTC meetings this came up at was also
to undeprecate existsSync() but keep exists() deprecated.
See: #8242 &
#8330

(Description write-up by @Fishrock123)

Fixes: #1592
Refs: #4217
Refs: #7455
PR-URL: #8364

Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@stevengill stevengill mentioned this issue Mar 8, 2017
3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.