fs: add recursive option to rmdir() by cjihrig · Pull Request #29168 · nodejs/node · GitHub

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

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

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.

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?

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

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

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

EDIT: CI was yellow.

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.

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

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

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>

Landed in 53816cc

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?

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.

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>

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

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.

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

10.x is in maintenance already so I think we missed the boat to do another semver minor

…

On Mon, May 11, 2020, 11:02 AM silverwind ***@***.***> wrote: Any chance of a backport to v10 for this (asking for sindresorhus/del#124 <sindresorhus/del#124>)? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#29168 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADZYV26DAS6E6RI276YBYTRRAHQXANCNFSM4IMMFUTA> .

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

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 16, 2019

bcoe mentioned this pull request Aug 17, 2019

fs: implement fs.rmdir recurisve #28171

Closed

4 tasks

cjihrig force-pushed the rimraf branch 2 times, most recently from 97b7a02 to e293b5d Compare August 19, 2019 19:07

cjihrig marked this pull request as ready for review August 19, 2019 19:09

silverwind reviewed Aug 19, 2019

View reviewed changes

Comment thread lib/internal/fs/utils.js Outdated

jasnell approved these changes Aug 19, 2019

View reviewed changes

silverwind added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 19, 2019

silverwind approved these changes Aug 19, 2019

View reviewed changes

Trott reviewed Aug 20, 2019

View reviewed changes

Comment thread doc/api/fs.md Outdated

Trott reviewed Aug 20, 2019

View reviewed changes

Comment thread doc/api/fs.md Outdated

Trott reviewed Aug 20, 2019

View reviewed changes

Comment thread doc/api/fs.md Outdated

cjihrig force-pushed the rimraf branch from 2a2fbd8 to 28dab64 Compare August 20, 2019 15:52

cjihrig added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Aug 20, 2019

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 20, 2019

bcoe approved these changes Aug 20, 2019

View reviewed changes

Comment thread doc/api/fs.md Outdated

Comment thread lib/internal/fs/rimraf.js Outdated

Comment thread lib/internal/fs/rimraf.js Outdated

Comment thread test/parallel/test-fs-rmdir-recursive.js Outdated

MylesBorins reviewed Aug 20, 2019

View reviewed changes

Comment thread lib/internal/fs/rimraf.js Outdated

cjihrig force-pushed the rimraf branch from a93476b to a46d568 Compare August 20, 2019 19:17

Trott approved these changes Aug 20, 2019

View reviewed changes

cjihrig mentioned this pull request Aug 20, 2019

test: use fs rimraf to refresh tmpdir #29235

Closed

3 tasks

jasnell approved these changes Aug 20, 2019

View reviewed changes

gengjiawen approved these changes Aug 21, 2019

View reviewed changes

Trott closed this Aug 23, 2019

cjihrig deleted the rimraf branch August 23, 2019 21:36

iansu mentioned this pull request Aug 23, 2019

lib: port rimraf to core as shutil.rmtree #28208

Closed

5 tasks

BridgeAR mentioned this pull request Sep 3, 2019

v12.10.0 proposal #29429

Merged

iansu mentioned this pull request Sep 6, 2019

Implement shutil in Node.js core nodejs/tooling#23

Closed

SimonSchick mentioned this pull request Sep 16, 2019

Add note that module is now shipped in node by default. isaacs/rimraf#204

Closed

richardlau mentioned this pull request Nov 7, 2019

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

Closed

ntwb mentioned this pull request Nov 28, 2019

Package: Replace fs-extra with rimraf. WordPress/gutenberg#18790

Merged

silverwind mentioned this pull request May 11, 2020

Cannot delete dirs containing files with invalid UTF8 sequences in their name sindresorhus/del#124

Closed

RyanZim mentioned this pull request Jun 3, 2020

Recursive fs.rmdir / fs.rm jprichardson/node-fs-extra#806

Closed

bcoe mentioned this pull request Jul 11, 2020

Rethink recursive flag (and design more generally) for fs.rmdir #34278

Closed

bcoe mentioned this pull request Sep 13, 2020

fs: remove experimental language from rmdir recursive #35171

Closed

4 tasks

aduh95 mentioned this pull request Jun 2, 2021

fs: stop fixing EPERM on Windows #38810

Closed

live627 mentioned this pull request Jan 23, 2022

Consider replacing del with its dependency rimraf avajs/ava#2952

Closed

fs: add recursive option to rmdir() by cjihrig · Pull Request #29168 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: add recursive option to rmdir() - #29168

fs: add recursive option to rmdir()#29168cjihrig wants to merge 3 commits intonodejs:masternodejs/node:masterfrom cjihrig:rimrafcjihrig/node:rimrafCopy head branch name to clipboard

Conversation

cjihrig commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

Trott commented Aug 18, 2019

Uh oh!

silverwind commented Aug 19, 2019

Uh oh!

cjihrig commented Aug 19, 2019

Uh oh!

nodejs-github-bot commented Aug 19, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Aug 20, 2019

Uh oh!

nodejs-github-bot commented Aug 20, 2019 • edited by cjihrig Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Trott commented Aug 20, 2019

Uh oh!

nodejs-github-bot commented Aug 21, 2019

Uh oh!

Trott commented Aug 23, 2019

Uh oh!

ljharb commented Aug 24, 2019

Uh oh!

Trott commented Aug 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mysticatea commented Sep 4, 2019

Uh oh!

silverwind commented May 11, 2020

Uh oh!

MylesBorins commented May 11, 2020 via email

Uh oh!

silverwind commented May 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

fs: add recursive option to rmdir()#29168
cjihrig wants to merge 3 commits into
nodejs:masterfrom
cjihrig:rimraf

cjihrig commented Aug 16, 2019 •
edited
Loading

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

Trott commented Aug 24, 2019 •
edited
Loading