Remove: Option for Error on Missing File or Directory

[CxRes]

As discussed in #246, this is a PR to allow for error message on ENOENT. I have provisionally only fixed remove() by adding a options.shoutMissing flag which is false by default.

I have made the changes defensively, since this is a library to fix the quirks of node fs and I am worried about breaking stuff. However, I have made more aggressive suggestions in the comments along side. Please examine and test it (I do not have access to all environments) and I'll make a proper PR according to your feedback.

[RyanZim]

Given that we'll probably be aiming for parity with Node core's new recursive option for fs.rmdir in the coming releases (see #806); I'd prefer not to add this additional option.

@manidlou @JPeer264 If you disagree, please reopen.

[CxRes]

@RyanZim I am unable to understand your logic. The whole point of this library is to provide file-system functionality that node is unable to. I could use node's API if I just wanted parity!

And it seems from the discussion in #246 that this is a popular request that many people have asked for. The reason offered for non-inclusion, quite incorrectly I might add, was that such a change would be breaking! This is NOT a breaking change!

[RyanZim]

The whole point of this library is to provide file-system functionality that node is unable to.

Until Node v12.10.0; there was no way to remove a directory with contents. So https://www.npmjs.com/package/rimraf was created to fill that gap. @jprichardson made this library because he got tired of installing rimraf and other similar utility modules on every project, so he bundled them all in this library. We now use a forked/vendored version of rimraf, but the codebase hasn't deviated much.

Now, finally Node core is beginning to add some of these common tasks to the core fs module. The recursive option for fs.rmdir is literally the code of rimraf being vendored in Node core. When that option is fully supported in all supported versions of Node (that'll be a few years from now), remove() won't have a reason to exist, since it will be providing the exact same functionality. We'll probably keep it as an alias for recursive rmdir for awhile, to allow people time to transition, but long-term, I don't see remove() being part of fs-extra. However, if we add this option, we'll be in an odd state where we'll be forced to keep a method that mirrors core by default, but has an extra option that changes behaviour. For that reason, I am opposed to adding this as an option to the current method.

---

In regards to adding this functionality in general (i.e. via a new method):

this is a popular request that many people have asked for

Looking at #246, if I counted right, there's a grand total of 6 people in favour of it. For a package that's depended on by nearly 4 million other packages, I'm not convinced it qualifies as a popular request. If we added every request that 6 developers asked for, we'd quickly end up with a massive, bloated, & complicated library. That said, I could be making the wrong call here. As I've stated before, if any of the maintainers disagree, I'd be happy to hear their point of view.

---

Thinking about this discussion, it does bring up an interesting point in my mind; it is a bit odd the way this works in Node core. Quoting Node docs:

In recursive mode, errors are not reported if path does not exist, and operations are retried on failure.

There'd be a much stronger argument to say that Node core should not swallow ENOENT errors, from a standpoint of consistency. It's a bit odd that a single option makes a method both recursive and ignore ENOENT. From a perspective of developer use, ignoring ENOENT is IMO the most common desired outcome, but from a consistency perspective, it's odd. *nix rm has separate options for this, -r & -f (rm -rf is just shorthand for rm -r -f). -r makes it recursive, -f makes it swallow ENOENT.

Might not be a dumb idea to open to open an issue on Node itself about this. If Node core changed functionality/added an option, my opinion on supporting this in fs-extra would change.

[CxRes]

@RyanZim Thanks for a very detailed reply. I am more sympathetic to your thought process. But I still have to disagree. Let me see if I can explain my POV better:

fse.remove in some sense is to node what rm would be to posix. Both combine the functionality of unlink and rmdir. AFAICT the node API is essentially modeled after the posix API. Heck, they even link directly to linux documentation for most functions including unlink and rmdir.

Now, I do not see how you can get rid of fse.remove given that the target, much like that for rm, can also be a file. The benefit of remove is that one need not know the state of the file system before requesting the operation. A recursive fs.rmdir still suffers from this problem and will force the check back to user land. If rm can inform of ENOENT if we want, imho fse.remove should too. I would be open to changing the form so that it is more rm like!

In any case, I would also welcome more input; we all want the best possible outcome!!!!

Now to some of your comments:

Thinking about this discussion, it does bring up an interesting point in my mind; it is a bit odd the way this works in Node core.

I think the way node is approaching this is wrong. Having followed posix conventions all along, they seem to have suddenly abandoned it with these poorly thought out flags on rmdir. I shall open an issue with node sometime next week, though I am not sure how to broach this issue.... from the point of view of flag removal or adding an rm equivalent? Suggestions?

if I counted right, there's a grand total of 6 people in favour of it...

With all due respect, I cannot help but chuckle! You speak of gazillions of users, I submit to you that there have ever been only 500 odd issues filed and 300 PRs (and much fewer of that would be regarding feature requests). Very few users ever speak up and even fewer submit changes. Sure, popular may be overstating it, but in the context of those numbers, my "grand total" of 6 people over the span of a couple of years speaking up on the same matter might not just be incidental or accidental. Statistics can be used to trivialize anything!

If we added every request that 6 developers asked for, we'd quickly end up with a massive, bloated, & complicated library.

Of course, I share your aversion to adding bloat but at the same time I believe that one should strive for feature completeness as well. It is difficult balance. However, in this case, a comparison with rm (which was made perhaps 25 years ago or more) indicates that this might not be a case of bloat.

[RyanZim]

You're correct that so far, Node has mostly be modelled after POSIX. The logic being implemented for recursive rmdir is functionally the same as our remove. That is, it will work even if the target is a file. Now, it's clear that doesn't make much sense. fs.rmdir(filename, { recursive: true }) does not convey intent properly at all. Effectively, recursive option turns rmdir into rm -f. The more I think about this, the more I am convinced the current direction of Node's naming does not make any sense. Currently considering opening an issue with Node myself.

Re: your comments about statistics, fair points. Not 100% sure I agree, but haven't given the matter deep thought. Regardless, that's neither here nor there ATM, since I won't be moving forward here until I get clarity on what Node is doing/thinking.

[CxRes]

Fair enough! Lets see what the folks at Node have to say....

[CxRes]

@RyanZim I finally remembered to file that issue!!! Hope its OK that i took the liberty to link you in the discussion.

[RyanZim]

Yes, thanks. Was meaning to do it myself, but didn't make it a priority. Thanks for going ahead; commented my thoughts there.

[CxRes]

@RyanZim Node has now "fixed" recursive remove! The ball is in your court now...

[RyanZim]

@CxRes Please file an issue so we can discuss potential APIs for doing this.

[CxRes]

Please reopen #246!

[RyanZim]

Fresh thread filed: #840