Skip to content
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

An optional parameter that lets us delete folders that aren't empty directly #22686

Closed
ghost opened this issue Sep 4, 2018 · 14 comments
Closed
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.

Comments

@ghost
Copy link

ghost commented Sep 4, 2018

Until now in Node 8.x, we CANNOT directly delete a folder with the sub folders with files. I'm not sure whether we can offer such a function because this is a common behaviour to delete a folder that is NOT empty (Considering the performance, we can write our core codes at C++ layer, and call it through js aspect).

For we've got rmdirSync or rmdir, maybe we can add an optional parameter to choose whether we allow to remove sub files/folders for the parent folder itself or not (In order to be compatible with it, the default value should be false, this means when you remove a folder that isn't empty, error will be thrown out like what can see now), something like this following:

import { rmdirSync } from "fs";
rmdirSync('d:/tryme',true); // The second parameter will let you allow to delete a folder that isn't empty, the default value is false.

PS:I know that some 3-rd parties have implemented this, but it would be better inject it into the nodejs's fs module, which is very useful and pratical.

@Trott
Copy link
Member

Trott commented Sep 4, 2018

If we end up doing this, please no Boolean trap in the API signature. Use an options object instead.

@apapirovski
Copy link
Member

So basically rimraf? This is a pretty regular request around here. As far as an API — it should just be a separate function, much like mkdirp.

@richardlau
Copy link
Member

As far as an API — it should just be a separate function, much like mkdirp.

That's not how we implemented recursive mkdir (#21875) -- the current code on master uses an options parameter (see also discussions in #22302 and #22585).

@boneskull
Copy link
Contributor

I am working on a rimraf impl

@apapirovski
Copy link
Member

That's not how we implemented recursive mkdir (#21875) -- the current code on master uses an options parameter (see also discussions in #22302 and #22585).

I'm aware but that's also the reason it hasn't been released. Given the feature testing angle, it's almost certain to land in an actual release as a separate function.

@ghost
Copy link
Author

ghost commented Sep 5, 2018

Thanks all for your suggestions!
And waiting for you changes for that.

@boneskull
Copy link
Contributor

it's almost certain to land in an actual release as a separate function.

Was a decision made on this? It seemed to be still under consideration...

@apapirovski
Copy link
Member

apapirovski commented Sep 5, 2018

Was a decision made on this? It seemed to be still under consideration...

I'm interpolating from the available data points... 😆

Someone should just open the PR at this point. I will later this week if no one gets to it before then.

@richardlau richardlau added fs Issues and PRs related to the fs subsystem / file system. feature request Issues that request new features to be added to Node.js. labels Sep 7, 2018
@shisama
Copy link
Contributor

shisama commented Nov 9, 2018

I had been waiting for the PR someone open. However, it seems not to be opened. So, I implemented on #24252
Please mention to me if someone open the PR implementing this.

@silverwind
Copy link
Contributor

So the conclusion of #24252 is that doing it in C++ was not fast enough? Should a future attempt at this feature still be in C++ or would we be open to a JS-only solution too?

@shisama
Copy link
Contributor

shisama commented May 22, 2019

So the conclusion of #24252 is that doing it in C++ was not fast enough?

Yes. #24252 implementation was not faster than rimraf because the implementation in C++ deletes directories sequentially. rimraf does in parallel.

@silverwind
Copy link
Contributor

I guess we could make an attempt at porting rimraf to core, including its retry logic. @boneskull I see you mentioned attempting it, did you get anywere?

@iansu
Copy link
Contributor

iansu commented Jul 24, 2019

This is currently in progress here: #28208

@richardlau
Copy link
Member

Implemented in #29168.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants