Node-fs-extra: fse.remove() && fse.removeSync() don't throw error on ENOENT file

[kutyepov]

try {
fse.removeSync('/directory/that/does/not/exist/index.js');
} catch (error) {
//no exception will be thrown
}

Similarly,

fse.remove('/directory/that/does/not/exist/index.js', function(error) {
//error is null
console.error(error);
});

fse version 0.26.5

Is it expected behavior? Otherwise, I can help fixing it.

[jprichardson]

Is it expected behavior? Otherwise, I can help fixing it.

This is expected. Since you're trying to remove the file, why would you care if it doesn't already exist?

[kutyepov]

Thank you for quick response.

I would be just nice to throw an exception for debugging purposes e.g. if incorrect path was passed.

[tndev]

I would also suggest to add an ENOENT error.

There are situations where it is useful to get this information, e.g. when implementing a DAV based protocol or a RESTful interface, it is required to return an error if the client tries to delete a resource or collection that does not exits. As of that it is currently required to do a test if the file or directory exits before the fse.remove can be called. The race condition as described in the deprecated fs.exists would not be that problematic in the remove case, but it still would prefer not to check for the existence of the file in a separate call.

But the main point why I would suggest to add the ENOENT is that fse.remove would behave same way as fs.unlink.

[RyanZim]

Since you're trying to remove the file, why would you care if it doesn't already exist?

Good point!

This would be a huge breaking change.

If this functionality is really needed (I'm not convinced it is), it should be a separate method. Feel free to open a feature request for that if you are so inclined.

[CxRes]

Many years late to the discussion, could we add a loud ENOENT version under a flag ie options: { AnnounceMissingObject: true } . This won't be breaking in anyway afaict - all we need to do is change cb(null) to options.AnnounceMissingObject ? cb(err) : cb(null) after the ENOENT tests. I could PR this if you like and would not mind suggestions on a better flag name.

[RyanZim]

@CxRes What's the use-case? Seems like it's pretty much of a edge case, since you're the first person since 2016 to need this.

[tndev]

@RyanZim I for example also still prefer to catch the case where a directory that I want to delete does not exist, because in actually all of the cases when this error would occur in my workflow it would indicate that something went wrong (inconsistencies with the database, race conditions, failed correct transaction handling, ...) and consistent behavior with fs. But as this issue was closed as not to be fixed due to being a breaking change I switched away from using fse directly, and only exposed the functions I need through an own module and if needed changed their behavior to be consistent with the one of the fs module.

[CxRes]

@RaynZim I have a use case where the knowledge of whether something did not exist or was deleted would affect the state of the database tracking it. In case the resource does not exist, I can, for example, skip touching the database or log it differently.

There are two things to note:
This feature will NOT be a breaking change. Nothing shall break in userland!
And ENOENT is an error, the fact that you are trying to access something non-existent means that you are out of sync with the state of the system (even if the resultant state happens to match your needs), and user should be allowed, if they so desire, to be aware of this, ideally without having to make an unnecessary extra system call (one which this library is making anyway and then discarding that information).

[RyanZim]

OK, PR welcome for further discussion.

[CxRes]

@RyanZim Please look at the PR above when you can!

[RyanZim]

Discussion moved to https://github.com/jprichardson/node-fs-extra/issues/840.