Node: An optional parameter that lets us delete folders that aren't empty directly

[ghost]

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]

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

[apapirovski]

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]

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

That's not how we implemented recursive mkdir (https://github.com/nodejs/node/pull/21875) -- the current code on master uses an options parameter (see also discussions in https://github.com/nodejs/node/pull/22302 and https://github.com/nodejs/node/issues/22585).

[boneskull]

I am working on a rimraf impl

[apapirovski]

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]

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

[boneskull]

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]

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.

[shisama]

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]

So the conclusion of https://github.com/nodejs/node/pull/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]

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]

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]

This is currently in progress here: https://github.com/nodejs/node/pull/28208

[richardlau]

Implemented in https://github.com/nodejs/node/pull/29168.