Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

fs.existsSync should not throw and catch an exception in the implementation #8731

Closed
MetaMemoryT opened this issue Nov 15, 2014 · 6 comments
Closed

Comments

@MetaMemoryT
Copy link

fs.execSync should be implemented in a manner that does not require throwing and catching an exception. Often while debugging a useful strategy is to break on all exceptions when searching for a problem. Having this function throw an exception just means more noise to wade through.

@cjihrig
Copy link

cjihrig commented Nov 16, 2014

@MetaMemoryT could you provide a code sample to clarify what you would like to happen? It's pretty common for node to throw on bad inputs. It's even more common to throw in synchronous methods.

@MetaMemoryT
Copy link
Author

The current implementation looks like this:

fs.existsSync = function(path) {
  try {
    nullCheck(path);
    binding.stat(pathModule._makeLong(path));
    return true;
  } catch (e) {
    return false;
  }
}

It would be nice if it looked like this:

fs.existsSync = function(path) {
  binding.existsSync(pathModule._makeLong(path));
}

and then a new ffi method was created, binding.existsSync that would not internally throw and catch a javascript exception. This would improve performance also, if it was implemented properly.

@cjihrig this is not a matter of throwing an exception on bad inputs, any path is a good input, the goal of this function is to return a Boolean representing whether or not the path exists. Throwing an exception is redundant, as it is representing the same information that this particular function is returning (a Boolean false value).

@rlidwka
Copy link

rlidwka commented Nov 16, 2014

Often while debugging one must break on all exceptions when searching for a problem.

Hmm... is there a way to do that with node.js cli?

@MetaMemoryT
Copy link
Author

with https://www.npmjs.org/package/node-inspector you can.

@cjihrig
Copy link

cjihrig commented Nov 16, 2014

@MetaMemoryT ah, you're referring to existsSync() - the original issue referenced execSync().

exists() and existsSync() are well documented as "black sheep." There is work going on to replace them. See #8418 and #8714.

This can probably be closed.

@MetaMemoryT MetaMemoryT changed the title fs.execSync should not throw and catch an exception in the implementation fs.existsSync should not throw and catch an exception in the implementation Nov 16, 2014
@MetaMemoryT
Copy link
Author

Oops, thanks.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants