Repository navigation
Rethink recursive flag (and design more generally) for fs.rmdir #34278
Description
Is your feature request related to a problem? Please describe.
We (@me and @RyanZim) have concerns regarding API direction for fs.rmdir flags.
Up till now node's fs API was modeled closely to POSIX. The new flags for fs.rmdir, in particular, the recursive flag is a break from this tradition. It is especially odd that rmdir can delete a file. Other difficulties include not knowing if you are deleting a file/folder that does not already exist because errors are not reported in recursive mode (at least users should be able to opt out of this behaviour).
We refer you to our discussion at fs-extra which builds on fs (subsequent to the PR being closed).
jprichardson/node-fs-extra#785
Describe the solution you'd like
A fs.rm consistent with POSIX rm. This would mean fs.rm will have the recursive semantics instead of fs.rmdir which will remain a lower level function.
Describe alternatives you've considered
One alternative would be to implement recursive semantics in user land, though I personally feel that we should use/expose OS API's maximally through node. We are open to other suggestions. This is a good time to address this issue while the feature is still experimental!
Activity
richardlau commentedon Jul 9, 2020
I have vague recollections that something similar may have been mentioned during the recent OpenJS World Collaborator summit during the Tooling group's session cc @nodejs/tooling
richardlau commentedon Jul 9, 2020
cc @nodejs/fs as well
RyanZim commentedon Jul 9, 2020
@CxRes Thanks for opening this, I've been meaning to do this, but didn't make it a priority. I agree with @CxRes on his points, but would just like to add a bit more clarity.
For me the biggest problem with the current system is that
recursiveis a magic flag that does a lot of things:rm -r(this is consistent with its naming; if this was all it did, I'd be mostly fine with it).ENOENTif the path does not exist, somewhat analogous torm -f.retryDelayoption.Each of these things could well be their own separate option, and the error-swallowing and retry options would be equally useful for non-recursive
fs.rmdir()andfs.unlink(). To be clear, I am not advocating for adding error-swallowing and retry options to plainrmdirandunlink; I am merely pointing out that it would be logically consistent to do so. In reality, bothrmdirandunlinkare simple POSIX methods (untilrecursivewas added), and I think it's logically consistent that they stay this way.For that reason,
rimraflogic should be its own method. I don't care much about naming;rmis what @CxRes suggested; has the potential disadvantage of being associated with plainrm, when it's actually closer torm -rf; I thinkremoveTree(or something similar) was also suggested in past discussions, before it was decided to make it an option. I will throw out my vote against naming itfs.rmrf, since it's non-intuitive for those not familiar with *nix shell. Also, being a maintainer of the npm modulefs-extra, I hopefs.removeisn't chosen, since that will create a naming conflict with the existingfs-extramethod and make my life a bit painful (regardless of the merits of that name).Trott commentedon Jul 10, 2020
cc @bcoe
iansu commentedon Jul 10, 2020
I made a PR #28208 to add
rimrafto Node.js as part of the Tooling Group. Originally we had it in a new module and named the functionrmtree. The new module idea was not well received so I moved it into thefsmodule under the same name.I was opposed to adding it as a flag to
rmdirand I agree that it is confusing thatrmdiralso deletes files. In the shellrmdirandrmare two different things. Using a flag also make feature detection more difficult. I was not involved with the PR that ultimately made it into core.The Tooling Group is discussing making more
fsAPIs recursive so I agree that it would be nice to reach some kind of consensus on this and establish a pattern.boneskull commentedon Jul 10, 2020
a few thoughts:
mkdir recursive also deviates from the 1:1 syscall.
an
allowRetriesflag which defaults to true on win32 seems reasonableif a user expects the directory to already be empty, then the recursive flag isn’t needed. I know you know this, but I can count the times in my long and uneventful career when I’ve needed to remove a tree of empty directories on one hand. that’s the most common thing people want to do, which is why it removes files.
there’s nothing explicitly stated in the docs that the functions in fs are intended to mirror syscalls (unless my reading comprehension really is that bad). if we want to do that, then we should make it explicit. but first we should absolutely ask ourselves why we’re doing it other than theoretical purity
@RyanZim I don’t understand if rmdir (as it stands) is problematic for fse. It didn’t sound like that was the main reason the original PR was closed—is that correct?
renaming it to “prune” or “rmtree” might be ok.
rmcould work as well, if it works on non-directories. we should then implement all of gnu coreutils 🙂imo, trying to adhere closely to posix-isms is not helpful given most people using node day-to-day use it on windows (afaik). just because that’s the way we’ve done it before
CxRes commentedon Jul 10, 2020
@boneskull Thanks for chiming in!!! Just to clarify a couple of points in response to your thoughts:
Re file deletion: its (at least in my mind) counter-intuitive that
fs.rmdirdeletes a file at the root (not that it deletes files inside some directory along with that directory) iefs.rmdirruns successfully to delete no directory at all. Its more confusing than wrong per se!I agree with you that adhering to POSIX conventions may not be the best way ahead - I think of it is as more of convenient crutch to circumvent the difficult discussions of API design where often there are no right answers. And I implicitly picked it up while making my initial suggestion. But I believe that any solution must allow users to efficiently reap the full benefits of the underlying OS APIs and not unnecessarily force operations to user land.
Trott commentedon Jul 10, 2020
Not saying it's wise or unwise, or even something we've continued to actually do, but the
fsdocs do say:coreyfarrell commentedon Jul 10, 2020
In the case of the default options I think this is still (mostly?) true. I don't have any objection to accepting an option to deviate from the normal mode of operation. I also don't object to moving recursive functions to separate names, though this will increase the number of functions exported by
fsandfs/promises. One concern I have is that when we propose recursive variants of additional functions we'll face resistance to further expansion of the namespaces.RyanZim commentedon Jul 10, 2020
rmdiras it stands is not problematic for fse; the discussion just clarified in my mind that therecursiveoption is a clumsy, unintuitive design, for anyone using Node.I didn't highlight this well in my earlier post; to be perfectly clear, you can delete a file with
fs.rmdir(file, { recursive: true }). Now that's unintuitive; but you're incentivised to write code like that, becausefs.unlinkwill throw if the file doesn't exist, and will throw if your antivirus happens to be scanning the file at the time (retries fix this).boneskull commentedon Jul 10, 2020
@Trott I stand corrected. maybe that should be removed as it’s no longer true.
there’s unfortunately nowhere else to put these recursive functions as adding a new builtin module (which is what we originally wanted to do, I think, and @iansu mentioned) would be a nonstarter for unrelated reasons.
iansu commentedon Jul 10, 2020
We were just discussing this issue in the Tooling Group meeting today. We do agree that being able to use
fs.rmdir(file, { recursive: true })to directly delete a file seems like a bug. That's something that should be easy to fix. If we were to do that would that alleviate some or all of the concerns people have raised here?RyanZim commentedon Jul 11, 2020
Correction on my earlier post, I said:
This is partially untrue. Retry logic is off by default (unlike
rimraf's behavior), by virtue ofmaxRetriesbeing0by default. However, themaxRetries&retryDelayoptions have no effect withoutrecursiveenabled, so retrying withoutrecursiveis impossible.boneskull commentedon Jul 11, 2020
if unlink can fail for the same reason on win32, perhaps that would want a maxRetries flag too. but I don’t see anything particularly heinous about an option that needs a second option to do anything. silently ignoring it is maybe a problem, but it just depends how much type validation node wants to be doing.
8 remaining items
fs: remove experimental from rmdir recursive
fs: remove experimental from rmdir recursive
fs: remove experimental from rmdir recursive
bcoe commentedon Oct 11, 2020
After discussions with the tooling group, and with the TSC, we ultimately did decide to introduce
fs.rm, which is modeled after POSIX'srm.We are however, not opting to remove the
rmdir/recursivebehavior immediately, as this would be too disruptive to users. Instead we're:rmdir/recursive, to indicates folks should expect bug fixes (and feel comfortable using the feature).fs.rmwhich we can direct users towards instead ofrmdir/recursive, if they want permissive behavior, i.e., for it not to throw on missing paths, or file paths.fs.rm.rmdir/recursive.node@16, we intend to throw on nonexistent paths, and file paths, when using thefs.rmdirmethod withrecursive=true.fs: remove experimental from promises.rmdir recursive
fs: remove experimental from promises.rmdir recursive
fs: remove experimental from promises.rmdir recursive
fs: remove experimental from promises.rmdir recursive
fs: remove experimental from promises.rmdir recursive
fs: remove experimental from rmdir recursive