Skip to content

/ node

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

Jump to bottom

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 •
edited

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
@bcoe bcoe mentioned this pull request Aug 17, 2019
Closed
4 of 4 tasks complete
@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.
@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.
@cjihrig cjihrig force-pushed the cjihrig:rimraf branch 2 times, most recently 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?
@nodejs-github-bot
Copy link

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

CI: https://ci.nodejs.org/job/node-test-pull-request/25108/
@silverwind
silverwind reviewed Aug 19, 2019
View changes
lib/internal/fs/utils.js Show resolved Hide resolved
@jasnell
jasnell approved these changes Aug 19, 2019
View changes
@silverwind
silverwind approved these changes Aug 19, 2019
View changes
@Trott
Trott reviewed Aug 20, 2019
View changes
doc/api/fs.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Aug 20, 2019
View changes
doc/api/fs.md Outdated Show resolved Hide resolved
@Trott
Trott reviewed Aug 20, 2019
View changes
doc/api/fs.md Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link

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

CI: https://ci.nodejs.org/job/node-test-pull-request/25124/
@cjihrig cjihrig force-pushed the cjihrig:rimraf branch Aug 20, 2019
@nodejs-github-bot
Copy link

@nodejs-github-bot nodejs-github-bot commented Aug 20, 2019 •
edited by cjihrig

CI: https://ci.nodejs.org/job/node-test-pull-request/25126/

EDIT: CI was yellow.
@bcoe
bcoe approved these changes Aug 20, 2019
View changes
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.
doc/api/fs.md Show resolved Hide resolved
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
lib/internal/fs/rimraf.js Show resolved Hide resolved
test/parallel/test-fs-rmdir-recursive.js Show resolved Hide resolved
@MylesBorins
MylesBorins reviewed Aug 20, 2019
View changes
lib/internal/fs/rimraf.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member

@Trott Trott commented Aug 20, 2019

We need to update license-builder.sh and re-run it?
@cjihrig cjihrig force-pushed the cjihrig:rimraf branch Aug 20, 2019
@Trott
Trott approved these changes Aug 20, 2019
View changes
@cjihrig cjihrig mentioned this pull request Aug 20, 2019
Closed
3 of 3 tasks complete
@jasnell
jasnell approved these changes Aug 20, 2019
View changes
@gengjiawen
gengjiawen approved these changes Aug 21, 2019
View changes
@nodejs-github-bot
Copy link

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

CI: https://ci.nodejs.org/job/node-test-pull-request/25137/
@cjihrig cjihrig force-pushed the cjihrig:rimraf branch to ee63d32 Aug 23, 2019
@nodejs-github-bot
Copy link

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

CI: https://ci.nodejs.org/job/node-test-pull-request/25205/
Trott added a commit that referenced this pull request Aug 23, 2019
@cjihrig @Trott
fs: add recursive option to rmdir()
Loading status checks…
53816cc 
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
@Trott Trott closed this Aug 23, 2019
@cjihrig cjihrig deleted the cjihrig:rimraf branch Aug 23, 2019
@iansu iansu mentioned this pull request Aug 23, 2019
Closed
2 of 5 tasks complete
@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?
@Trott
Copy link
Member

@Trott Trott commented Aug 24, 2019 •
edited

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.
BridgeAR added a commit that referenced this pull request Sep 3, 2019
@cjihrig @BridgeAR
fs: add recursive option to rmdir()
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
ed2293e 
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
Merged
BridgeAR added a commit that referenced this pull request Sep 3, 2019
@BridgeAR
2019-09-04, Version 12.10.0 (Current)
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
471f4a9 
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 pull request Sep 4, 2019
@BridgeAR
2019-09-04, Version 12.10.0 (Current)
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
3a2e75d 
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 pull request Sep 4, 2019
@BridgeAR
2019-09-04, Version 12.10.0 (Current)
Verified
This commit was signed with a verified signature.
BridgeAR Ruben Bridgewater
GPG key ID: F07496B3EB3C1762 Learn about signing commits
Loading status checks…
02c74e7 
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.
@iansu iansu mentioned this pull request Sep 6, 2019
Closed
@SimonSchick SimonSchick mentioned this pull request Sep 16, 2019
Open
@richardlau richardlau mentioned this pull request Nov 7, 2019
Closed
@ntwb ntwb mentioned this pull request Nov 28, 2019
Merged
@silverwind
Copy link
Contributor

@silverwind silverwind commented May 11, 2020

Any chance of a backport to v10 for this (asking for sindresorhus/del#124)?
@silverwind silverwind mentioned this pull request May 11, 2020
Closed
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented May 11, 2020

@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.
@RyanZim RyanZim mentioned this pull request Jun 3, 2020
Open
@bcoe bcoe mentioned this pull request Jul 11, 2020
Closed
@bcoe bcoe mentioned this pull request Sep 13, 2020
Closed
4 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@silverwind silverwind

@bcoe bcoe

@MylesBorins MylesBorins

@vsemozhetbyt vsemozhetbyt

@jasnell jasnell

@Trott Trott

@gengjiawen gengjiawen

Assignees
No one assigned
Labels
author ready lib / src notable-change semver-minor
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.