-
Notifications
You must be signed in to change notification settings - Fork 30k
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: implement fs.rmdir recurisve #28171
Conversation
Add `clearDir` options into fs.rmdir and fs.rmdirSync to delete a forder with sub folders or files.
IMO, |
Hello @zero1five and thanks for the contribution! The subject of recursive rmdir (a.k.a. rimraf) has also been is discussion by the @nodejs/tooling team - nodejs/tooling#13. A few points that were raised:
Work is also in progress on these ideas by by @iansu. |
imo, prefer incremental implementation requirements. @refack Should I move on or shut it down? await node/tooling's idea. |
@zero1five it's great to see other folks from the community excited about this feature. We had a good discussion about
I'd love to hear what other folks think (don't see any reason to close this immediately). |
To expand on
Also, robust removal has to deal with the fact that background Windows processors (virus scanners, commonly) open files at random times, and lock files as a side-effect, preventing them from being removed. Its a big problem to get removal robust on Windows, with lots of heuristics involved (exponential backoff and retry), which makes it very, very unlike every other API in fs, which are mostly implemented 1-1 with some underlying Windows or Posixy system call, and therefor have fairly straightforward to understand behaviour. |
Could the option be named more clearly? |
@zero1five would you be open to combining efforts with @iansu here: Ian has been working on this patch with @isaacs', who wrote rimraf; together they've been trying to make sure that all of the edge-cases brought up by @sam-github are appropriately addressed (if there were to be a Node.js implementation of the library). |
@bcoe I'd be pleased to come. |
Maybe |
Yes, I like |
Perhaps fs.rmdir(path, { recursive: true, emptyOnly: false }) |
@jasnell I don’t think it makes sense to specify So it seems like what you’re suggesting in the end is to name the option |
👋 @zero1five I'm going to close this, because it seems like we're reaching some consensus around choosing to vendor the existing rimraf module. With two approaches being put forward, either introducing a recursive flag to or exposing a new method on fs: Let's try to move the conversation to these PRs and see if we can keep this moving forward 🎉 |
Fwiw, although delete-empty isn't broadly used, I've learned that deleting "empty" directories is more nuanced that it seems at first, and (IMHO) it's probably not something we'd want to handle natively in Node.js. For example, on macs, it's often debatable whether or not a directory is actually empty, since MacOS sometimes adds a hidden Anyway, just a thought, I know this issue is closed but figured I should comment in case the discussion arises again. This is one of those things that seems really straightforward but (IMHO) isn't. |
Add
clearDir
options into fs.rmdir and fs.rmdirSync to delete a forder with sub folders or files. This PR is an attempt to portrimraf
into the core from the js layer.Refs: #22686
Refs: #27764
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes