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

fs: add recursive option to rmdir() #29168

Closed
wants to merge 3 commits into from
Closed

fs: add recursive option to rmdir() #29168

wants to merge 3 commits into from

Conversation

@cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 16, 2019

This PR adds a recursive option to fs.rmdir(), fs.rmdirSync(), and fs.promises.rmdir(). The implementation is a port of the npm module rimraf. I added an option to rmdir() to match the approach taken with core's recursive mkdir().

This is my alternative to #28208 from #28208 (review).

I'm marking this PR as a work in progress, as I haven't added docs yet.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
@Trott
Copy link
Member

@Trott Trott commented Aug 18, 2019

FWIW, other than the docs and tests that need to be added, this looks good to me.

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented Aug 19, 2019

Liking this as well, good call with the lazy loading. Waiting for docs/tests as well. Maybe some of the edge cases like the Windows ones can be included in the tests as well, but it's not a strict requirement.

Loading

@cjihrig cjihrig force-pushed the rimraf branch 2 times, most recently from 97b7a02 to e293b5d Aug 19, 2019
@cjihrig cjihrig marked this pull request as ready for review Aug 19, 2019
@cjihrig
Copy link
Contributor Author

@cjihrig cjihrig commented Aug 19, 2019

OK, this is ready for review.

It occurred to me that we may want to mark this as experimental for a few releases. Any thoughts?

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 19, 2019

Loading

lib/internal/fs/utils.js Show resolved Hide resolved
Loading
doc/api/fs.md Outdated Show resolved Hide resolved
Loading
doc/api/fs.md Outdated Show resolved Hide resolved
Loading
doc/api/fs.md Outdated Show resolved Hide resolved
Loading
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 20, 2019

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 20, 2019

Loading

bcoe
bcoe approved these changes Aug 20, 2019
Copy link
Member

@bcoe bcoe left a comment

Having thought about rimraf way too much over the last few months, I ultimately come around to liking the consistency that a recursive option introduces (in relation to our implementation of mkdir recursive)...

I think there's meat to the argument that this makes rmdir with an option combine both unlink and rmdir behavior, which is a bit weird, but I feel this is ultimately the better user experience.

Loading

doc/api/fs.md Show resolved Hide resolved
Loading
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
Loading
lib/internal/fs/rimraf.js Show resolved Hide resolved
Loading
test/parallel/test-fs-rmdir-recursive.js Show resolved Hide resolved
Loading
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
Loading
@Trott
Copy link
Member

@Trott Trott commented Aug 20, 2019

We need to update license-builder.sh and re-run it?

Loading

Trott
Trott approved these changes Aug 20, 2019
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 21, 2019

Loading

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 23, 2019

Loading

Trott added a commit that referenced this issue Aug 23, 2019
This commit adds a recursive option to fs.rmdir(),
fs.rmdirSync(), and fs.promises.rmdir(). The implementation
is a port of the npm module rimraf.

PR-URL: #29168
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@Trott
Copy link
Member

@Trott Trott commented Aug 23, 2019

Landed in 53816cc

Loading

@Trott Trott closed this Aug 23, 2019
@cjihrig cjihrig deleted the rimraf branch Aug 23, 2019
@ljharb
Copy link
Member

@ljharb ljharb commented Aug 24, 2019

Sorry i missed the PR; nobody posted a comment in #28208 indicating this one was opened.

Does this mean that to feature detect this option, i have to try/catch around passing the wrong type of options object into the sync function? Am i missing a less invasive mechanism?

Loading

@Trott
Copy link
Member

@Trott Trott commented Aug 24, 2019

Sorry i missed the PR; nobody posted a comment in #28208 indicating this one was opened.

Does this mean that to feature detect this option, i have to try/catch around passing the wrong type of options object into the sync function? Am i missing a less invasive mechanism?

Correct. It's the same with the recursive option for mkdir(). We had a lot of discussion around feature detection for that, but when it came down to it, people either didn't need feature detection, kept using the mkdirp package, or did sniffing on process.version/process.versions.node . Not everyone was thrilled about version sniffing but no solution was going to make everyone happy (as the rest of that issue thread and others make clear) and it seems to be working out OK.

Loading

BridgeAR added a commit that referenced this issue Sep 3, 2019
This commit adds a recursive option to fs.rmdir(),
fs.rmdirSync(), and fs.promises.rmdir(). The implementation
is a port of the npm module rimraf.

PR-URL: #29168
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Sep 3, 2019
BridgeAR added a commit that referenced this issue Sep 3, 2019
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
BridgeAR added a commit that referenced this issue Sep 4, 2019
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
BridgeAR added a commit that referenced this issue Sep 4, 2019
Notable changes:

* deps:
  * Update npm to 6.10.3 (isaacs)
    #29023
* fs:
  * Add recursive option to rmdir() (cjihrig)
    #29168
  * Allow passing true to emitClose option (Giorgos Ntemiris)
    #29212
  * Add \*timeNs properties to BigInt Stats objects (Joyee Cheung)
    #21387
* net:
  * Allow reading data into a static buffer (Brian White)
    #25436

PR-URL: #29429
@mysticatea
Copy link

@mysticatea mysticatea commented Sep 4, 2019

Does this mean that to feature detect this option, i have to try/catch around passing the wrong type of options object into the sync function?

I guess fs.rmdirSync.length can be used to detect because the number of arguments was changed.

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented May 11, 2020

Any chance of a backport to v10 for this (asking for sindresorhus/del#124)?

Loading

@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented May 11, 2020

Loading

@silverwind
Copy link
Contributor

@silverwind silverwind commented May 11, 2020

Fine with me. Agree it would be odd to add features like this so late in the cycle.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet