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

Add fs.exists and fs.existsSync if it doesn't exist. #145

Closed
jdalton opened this issue Jul 2, 2015 · 20 comments
Closed

Add fs.exists and fs.existsSync if it doesn't exist. #145

jdalton opened this issue Jul 2, 2015 · 20 comments

Comments

@jdalton
Copy link
Contributor

jdalton commented Jul 2, 2015

Since fs.exists and fs.existsSync are deprecated it would be good to check if they exist and add them if they don't. You could reference https://github.com/sindresorhus/path-exists.

@jprichardson
Copy link
Owner

I've considered this, but haven't acted because it's my understanding that despite the fact that they're being deprecated, they're still currently included in io.js and the deprecation message hasn't been enabled yet (or was disabled - nodejs/node#257). So I didn't feel any sense of urgency. Do you know if there's any discussion on actually removing the methods? Regardless, I suppose it wouldn't hurt to be ready.

Edit: related: #127

@jprichardson jprichardson modified the milestone: 1.0 Jul 2, 2015
@jdalton
Copy link
Contributor Author

jdalton commented Jul 2, 2015

Do you know if there's any discussion on actually removing the methods?

I do not.

Regardless, I suppose it wouldn't hurt to be ready.

Yap. I figure if it were added then when they do pull the trigger fs-extra would be ready.

@goloroden
Copy link

There was a reason why they were deprecated: They mislead developers to rely on them to ensure that a file exists, but they directly lead to a race condition.

Hence, it's basically a good thing that they have been deprecated. I'm not sure if fs-extra should re-enable them, with respect to the fact that there is an actual reason why they have been deprecated.

Wouldn't it be better to help spread the word on how to correctly deal with situations where you would use fs.exists? Maybe I'm missing something, so feel free to correct me if you think that I'm wrong… I was just thinking out loud ;-).

@jdalton
Copy link
Contributor Author

jdalton commented Jul 19, 2015

A doc note would do. I think it's handy and suspect the race condition concern doesn't apply to most devs using the method.

@jprichardson
Copy link
Owner

Wouldn't it be better to help spread the word on how to correctly deal with situations where you would use fs.exists? Maybe I'm missing something, so feel free to correct me if you think that I'm wrong… I was just thinking out loud ;-).

I think the core Node devs are wrong on this one. No doubt that they're right that it could lead to a race condition if you check if a file exists before opening it. But sometimes you just want to know if a file exists or not. Not because you want to read/write it, but because the mere existence may indicate the completion of some operation.

That being said, they're very useful methods and fs-extra will add them. However, I won't hide the original exists/existsSync methods, I'll need to come up with new names. I'll probably change the signature too from (doesExist) to (error, doesExist) so that this entire library can easily be promisified.

@goloroden
Copy link

@jprichardson What about isPresent as an alternative name for exists? The is would also indicate that its return value is boolean.

@acmello
Copy link

acmello commented Jul 21, 2015

so how's that so far? will it be added? I was working on a solution and this would be quite handy since I'm already using the fs-extra module but also, the deprecated fs.exists from fs. Btw, isPresent would be a good name :)

@jprichardson
Copy link
Owner

@acmello no work has commenced on this yet. It's not because it's difficult; this implementation should be pretty simple. Rather, it's because it's good to let issues like these marinate for the sake of discussion. To bikeshed a bit, what about doesExist() for a name?

@acmello
Copy link

acmello commented Jul 21, 2015

@jprichardson I was actually glad to see that you guys were talking about it. doesExist() is also a good one!

@guiprav
Copy link

guiprav commented Jan 21, 2016

@jprichardson, has this not marinated long enough by now? ;-) There are definitely legitimate uses for fs.exists. The core maintainers are dumbing down the core so programmers won't hurt themselves, like mommy and daddy do.

In case you're unconvinced, I use path-exists because in one of my projects I have a data file and module overriding system in place that works like this: If you want to load a data file, the overriding system will first try to load it from a directory that corresponds with the environment the application is currently targeting. If that file doesn't exist, it then tries to load that same file from a "common" directory. In other words, we have many files under common/, but if a file by the same name is present under staging/ and the application is targeting staging, it will load staging/<requested-file>, otherwise it loads from common/<requested-file>. Obviously, I need fs.exists for that.

Those data files and modules are versioned, part of the source code, so they are not going away and back all the time. They're created once before the application runs and are only changed after the application finishes running. Hence, no race conditions.

To give another, even simpler example, I saw a guy who's implementing a text editor, and the editor keeps a list of recently opened files. On startup, if a file from the list doesn't exist anymore, it gets removed. It could also have been temporarily hidden (to better cope with removable media, for example). Regardless, there's no race condition there.

These are the legitimate things fs.exists was meant for not only in Node, but on any other programming language as well. Granted, lots of people are going to misuse it, but as many people have pointed out to Node maintainers, you fix that with education (documentation).

Remember Ken Thompson: "I abhor a system designed for the ‘user’, if that word is a coded pejorative meaning ‘stupid and unsophisticated’."

Totally off-topic: @acmello, cheers, my 9th floor buddy! (To others: We work in the same building).

@jprichardson
Copy link
Owner

@jprichardson, has this not marinated long enough by now? ;-) There are definitely legitimate uses for fs.exists. The core maintainers are dumbing down the core so programmers won't hurt themselves, like mommy and daddy do.

I completely agree. This is something that I want to do. There are completely legitimate reasons for the existence of files helping to determine application state. .lock files are good examples.

I guess I don't understand the major concern as of yet, since fs.exists and fs.existsSync already exist. Anytime I use them in Node.js core v5, I don't get any deprecation warnings or issues - I know that they're deprecated.

Anyways, I'd be willing to accept a PR in accordance with @jdalton's original issue. That is, fs.exists and fs.existsSync should be implemented IF they don't already exist.

Please let me know if I'm missing anything or have a misunderstanding. Thanks!

@guiprav
Copy link

guiprav commented Jan 21, 2016

I agree, I'm glad you think that way.

You might want to consider always replace fs.exists in case you want to fix the original function signature issue (it's different from all other standard async functions, i.e. it has no err parameter and so result becomes the first one). If you haven't already, you can look up what's wrong with that. Personally, it doesn't bother me too much, I'm just raising this so you can make a conscious decision.

@ghost
Copy link

ghost commented Sep 27, 2016

I don't understand the reason behind making .exists() depracated, and frankly I don't care. I like using exists() because it is concise.

So, please add this function to fs-extra so that I can keep using it.

@jprichardson
Copy link
Owner

So, please add this function to fs-extra so that I can keep using it.

Is Node v7 removing it?

@jdalton
Copy link
Contributor Author

jdalton commented Oct 12, 2016

Just a heads up Node v6.8.0 undeprecated fs.existsSync though it still has fs.exists (the async form) deprecated.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 26, 2016

@jprichardson Should this be removed from the 1.0.0 milestone?

@jprichardson jprichardson removed this from the 1.0.0 milestone Oct 27, 2016
@jprichardson
Copy link
Owner

@jprichardson Should this be removed from the 1.0.0 milestone?

Done.

Given that fs.existsSync has been undeprecated and fs.exists is still hanging around in Node v7.0.0, this isn't a huge priority.

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 27, 2016

FWIW, we will probably revisit this when we promisify fs-extra.

@RyanZim
Copy link
Collaborator

RyanZim commented Apr 24, 2017

My proposal PR: #406

@RyanZim
Copy link
Collaborator

RyanZim commented Jun 21, 2017

Going to close this out; we'd suggest that people use pathExists() instead of exists().

@RyanZim RyanZim closed this as completed Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants