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

fs: deprecate exists() and existsSync() #166

Closed
wants to merge 1 commit into from
Closed

fs: deprecate exists() and existsSync() #166

wants to merge 1 commit into from

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 15, 2014

These methods don't follow standard conventions, and shouldn't be used anyway.

This closes #103

@@ -228,9 +228,9 @@ fs.exists = function(path, callback) {
function cb(err, stats) {
if (callback) callback(err ? false : true);
}
};
}, 'fs: exists() is deprecated.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other modules write it as module.function() and point the user to what to use instead (e.g. 'http.createClient is deprecated. Use http.request instead.'). I would suggest following that.

@bnoordhuis
Copy link
Member

It's probably a good idea to update most uses of fs.exists() in the test suit as well. The only exceptions I can think of are simple/test-fs-exists and maybe simple/test-fs-null-bytes.

These methods don't follow standard conventions, and shouldn't
be used anyway.
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 18, 2014

@bnoordhuis ready for review again.

@vtambourine
Copy link

Actually, accessSync looks misconventional as well. Only way to use this method is to wrap your code with try / catch block and catch error if provided path is not accessible. The possibility to store existence in variable might be expectable.

@bnoordhuis
Copy link
Member

@cjihrig LGTM

@vtambourine I kind of like it. I hope it discourages the anti-pattern of checking if a file exists before trying to open it.

@vtambourine
Copy link

@bnoordhuis,

Can you please explain why this is considered as anti-pattern?

Besides, consider following situation. I want to ensure that set of particular files exists (set of configs, for example, server.config.js, client.config.js etc.). One way of doing that is to store the existence of each file in hash, then filter out all existing files and initialise rests with some default content. Does this solution uses anti-pattern?

@bnoordhuis
Copy link
Member

Can you please explain why this is considered as anti-pattern?

Because the way most people use it, it's a TOCTOU race condition: the file may have changed or have been deleted between checking and opening it. If you are going to open it, then you need to be prepared for such eventualities, so you might as well skip the check and open it straight away.

@vtambourine
Copy link

Ok, thank you.

cjihrig added a commit that referenced this pull request 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 Author

cjihrig commented Dec 19, 2014

Landed in 5678595

@cjihrig cjihrig closed this Dec 19, 2014
@cjihrig cjihrig deleted the deprecate-exists branch December 19, 2014 15:35
@cjihrig cjihrig mentioned this pull request Jan 12, 2015
boingoing pushed a commit to boingoing/node that referenced this pull request Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants