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

Deprecate fs.exists or fix its API #103

Closed
benjamingr opened this issue Dec 6, 2014 · 20 comments
Closed

Deprecate fs.exists or fix its API #103

benjamingr opened this issue Dec 6, 2014 · 20 comments
Assignees
Labels
fs Issues and PRs related to the fs subsystem / file system.

Comments

@benjamingr
Copy link
Member

fs.exists is infamous for having an inconsistent non-nodeback API that confuses new users often and can be a pain spot.

I see two good alternatives:

  • Deprecate it, mark it as deprecated in the docs and suggest fs.stat instead + discuss the inherent problem with using exists (race condition). Add a big warning. Optionally console.log a deprecation notice when the server is first started.
  • Change the API to return a nodeback. I like this idea less since it'd have to be written in a way that wouldn't break current code. This sounds harder and less optimal.

Personally I'm for the first. Let's clean up fs :)

@rvagg
Copy link
Member

rvagg commented Dec 6, 2014

Yes! Option 1 is my vote. This is a tiny black spot on the Node fs API.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 7, 2014

+1 for deprecating. This was already being discussed in nodejs/node-v0.x-archive#8418. fs.access() an alternative is also up for PR nodejs/node-v0.x-archive#8714. Since both of those are mine, I'd be glad to do the work here.

@ghostbar
Copy link
Contributor

ghostbar commented Dec 7, 2014

+1 on option 1.

@juliangruber
Copy link
Member

👍 for deprecating

@bnoordhuis
Copy link
Member

Sounds reasonable. @cjihrig Go for it.

@benjamingr
Copy link
Member Author

@cjihrig yes please! The fix looks pretty solid and so far everyone is for deprecating it.

@fengmk2
Copy link
Contributor

fengmk2 commented Dec 7, 2014

@cjihrig 👍 here

@indutny
Copy link
Member

indutny commented Dec 8, 2014

Summoning @caineio

@caineio
Copy link

caineio commented Dec 8, 2014

Hello!

I am pleased to see your valuable contribution to this project. Would you
please mind answering a couple of questions to help me classify this submission
and/or gather required information for the core team members?

Questions:

  1. Issue-only Does this issue happen in core, or in some user-space
    module from npm or other source? Please ensure that the test case
    that reproduces this problem is not using any external dependencies.
    If the error is not reproducible with just core modules - it is most
    likely not a io.js problem. Expected: yes
  2. Which part of core do you think it might be related to?
    One of: tls, crypto, buffer, http, https, assert, util, streams, smalloc, cluster, child_process, dgram, c++, docs, other (label)
  3. Which versions of io.js do you think are affected by this?
    One of: v0.10, v0.12, v1.0.0 (label)

Please provide the answers in an ordered list like this:

  1. Answer for the first question
  2. Answer for the second question
  3. ...

Note that I am just a bot with a limited human-reply parsing abilities,
so please be very careful with numbers and don't skip the questions!

In case of success I will say: ...summoning the core team devs!.

In case of validation problem I will say: Sorry, but something is not right here:.

Truly yours,
Caine.

Responsibilities

  1. indutny: crypto, tls, https, child_process, c++
  2. trevnorris: buffer, http, https, smalloc
  3. bnoordhuis: http, cluster, child_process, dgram

@piscisaureus piscisaureus added the wip Issues and PRs that are still a work in progress. label Dec 8, 2014
@benjamingr
Copy link
Member Author

  1. Yes
  2. fs
  3. v0.12

On Dec 8, 2014, at 15:30, Michael Caine notifications@github.com wrote:

Hello!

I am pleased to see your valuable contribution to this project. Would you
please mind answering a couple of questions to help me classify this submission
and/or gather required information for the core team members?

Questions:

Issue-only Does this issue happen in core, or in some user-space module from npm or other source? Please ensure that the test case that reproduces this problem is not using any external dependencies. If the error is not reproducible with just core modules - it is most likely not a io.js problem. Expected: yes
Which part of core do you think it might be related to? One of: tls, crypto, buffer, http, https, assert, util, streams, smalloc, cluster, child_process, dgram, c++, docs, other (label)
Which versions of io.js do you think are affected by this? One of: v0.10, v0.12, v1.0.0 (label)
Please provide the answers in an ordered list like this:

Answer for the first question
Answer for the second question
...
Note that I am just a bot with a limited human-reply parsing abilities,
so please be very careful with numbers and don't skip the questions!

In case of success I will say: ...summoning the core team devs!.

In case of validation problem I will say: Sorry, but something is not right
here:.

Truly yours,
Caine.

Responsibilities

indutny: crypto, tls, https, child_process, c++
trevnorris: buffer, http, https, smalloc
bnoordhuis: http, cluster, child_process, dgram

Reply to this email directly or view it on GitHub.

@indutny
Copy link
Member

indutny commented Dec 8, 2014

@caineio what's up with you? Why are you ignoring this?

@indutny
Copy link
Member

indutny commented Dec 8, 2014

Ah, gotcha. It was assigned!

@caineio
Copy link

caineio commented Dec 8, 2014

...summoning the core team devs!

@caineio caineio added caine: passed fs Issues and PRs related to the fs subsystem / file system. and removed wip Issues and PRs that are still a work in progress. need info labels Dec 8, 2014
cjihrig added a commit that referenced this issue Dec 19, 2014
These methods don't follow standard conventions, and shouldn't
be used anyway.

Fixes: #103
PR-URL: #166
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
@cjihrig
Copy link
Contributor

cjihrig commented Dec 19, 2014

fs.exists() and fs.existsSync() are deprecated as of 5678595

@cjihrig cjihrig closed this as completed Dec 19, 2014
@benjamingr
Copy link
Member Author

Awesome news :) Thanks a ton.

cjihrig added a commit that referenced this issue Jan 12, 2015
fs.exists() and fs.existsSync() were deprecated in #103. This
commit removes the deprecation message, in order to stay more
in sync with joyent/node for the io.js 1.0.0 release.

Fixes: #257
PR-URL: #307
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@matthew-dean
Copy link

I am sad at this deprecation.

@captn3m0
Copy link

captn3m0 commented May 3, 2015

We have an npm module now: https://github.com/sindresorhus/path-exists

@xixixao
Copy link

xixixao commented Oct 10, 2015

/facepalm How was this not sufficient:

fs.exists() should not be used to check if a file exists before calling fs.open(). Doing so introduces a race condition since other processes may change the file's state between the two calls. Instead, user code should call fs.open() directly and handle the error raised if the file is non-existent.

@matthew-dean
Copy link

Because I don't want to open the file.

@Fishrock123
Copy link
Member

Please see: #1592

@nodejs nodejs locked and limited conversation to collaborators Oct 10, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

No branches or pull requests