fs: add rm method by iansu · Pull Request #35494 · nodejs/node · GitHub

Based on the decision by the TSC in their October 1st meeting I'm opening this PR and closing my previous PR #35250 which kicked off this discussion. You can see a summary of the plan the TSC agreed on in this comment: #35250 (comment)

This PR introduces a new method fs.rm that provides the behaviour of rimraf when used with the recursive: true and force: true options. It also adds a deprecation warning when using fs.rmdir with recursive: true only if the path is a file or does not exist. This functionality has also been deprecated in the docs: #35171

The behaviour of this new fs.rm method follows that of the UNIX rm command. The exact behaviour is explained in this table:

Arguments	File	Directory	Non-existent
none	Deletes file	Error, is a directory	Error, doesn't exist
recursive	Deletes file	Deletes directory	Error, doesn't exist
force	Deletes file	Error, is a directory	No error
recursive, force	Deletes file	Deletes directory	No error

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

Just a small amount of initial review. I'll leave more through review, once a few more folks have chimed in.

love it 😄

I think we should use the async validation here, otherwise we're going to create a bottleneck when performing many rm operations, you can do something like this:

options = await new Promise((resolve, reject) => {
  validateRmOptionsSync(path, options, false, (err, options) => {
     if (err) return reject(err);
     else return resolve(options);
  });
})
return rimrafPromises(path, options);

isn't this still creating the same bottleneck tho, it's just deferring the result? a new Promise executor runs synchronously.

@ljharb my bad, I meant:

options = await new Promise((resolve, reject) => {
  validateRmOptions(path, options, false, (err, options) => {
     if (err) return reject(err);
     else return resolve(options);
  });
})
return rimrafPromises(path, options);

i.e., not using the sync version of the validation.

I implemented this a few days ago and I did catch that mistake in your suggested change @bcoe. I'm not sure if that changes the point @ljharb raised.

This hasn't been updated yet, but yes, @bcoe's updated suggestion would address my point. I'd also say that it's worth adding a custom promisify implementation to validateRmOptions, and then use await promisify(validateRmOptions)(path, options, false) instead?

Two points:

May I suggest we not use rm as the name here. This would keep rm available for the eventual recursive remove.
The eventual recursive remove/rm should ideally have all the semantics of POSIX rm

May I suggest we not use rm as the name here. This would keep rm available for the eventual recursive remove.

@CxRes I'm pretty confused at this point, @iansu is matching the behavior of the method rm, with:

recursive: indicating a recursive operation.
force: indicating that errors should not be thrown.

If the issue is that we're not actually using a binding to an os level rm for the implementation, to me this is an implementation detail, we could choose to change in the future, and has no baring on this feature from a user perspective.

@bcoe going back to the TSC points... 2 said we would set up a function that rmdir -r would alias to (ie this PR) and then 4 will introduce the proper recursive remove. What I am saying is that 4 being the permanent solution should be called rm (or at the very least we keep the rm namespace available) and this should be given a another name. Unless, that is, you intend not to do 4 at all, in which case one must remember to add all the new flags here.

@CxRes Point 2 from the TSC plan is already done. This PR is point number 4.

@iansu I only saw #35171 being closed thru 35b17d9 which only had the deprecation notice. Have you made changes elsewhere?

@CxRes this is an interpretation of the suggestions from the TSC meeting, that I believe could be a good approach:

1) Let's mark the current API as stable, since it's used a lot

This work has been done here.

2. Rename the current API to e.g., rm while keeping the current API as alias to the new one

This PR introduces the method rm, which if used as fs.rm(path, {recursive: true, force: true}) has the current behavior of fs.rmdir(path, {recursive: true}).

Since we have opted to use the rm method name, we made the force and recursive configurable.

which case one must remember to add all the new flags here

I like that using the analogy rm leaves us open to adding more flags over time, e.g., -I for an interactive mode. But we do not need to add all these flags at the outset, rather I would treat the rm API as a guide for additional options we could add.

3. The alias gets deprecated

This PR prints a deprecation warning if you attempt to use recursive to delete a file or missing directory.

4. Add a new function that has the stricter version

I am advocating that rather than adding another "stricter" method, we will make the existing recursive flag stricter, such that it starts to throw on missing paths, and files provided as arguments.

This will bring the behavior of the existing fs.rmdir(path, {recursive: true}) inline with other platforms, and folks will have the new method fs.rm if they want more permissive behavior.

I am advocating that rather than adding another "stricter" method, we will make the existing recursive flag stricter, such that it starts to throw on missing paths, and files provided as arguments.

OK! This, I guess, was the source of my confusion. I was interpreting this as a new method all along! My only question in that case is: Do you really want to bring back the deprecated recursive flag once it is dropped in node@v16 (since we now have rm). Could we just leave it dropped, esp. since we are again aligned with the POSIX scheme?

Edit: Never mind that, I read @bcoe's comment on the other thread. I understand what you are doing now. Thanks for taking all this trouble!!!

OK! This, I guess, was the source of my confusion. I was interpreting this as a new method all along! My only question in that case is: Do you really want to bring back the deprecated recursive flag once it is dropped in node@v16 (since we now have rm). Could we just leave it dropped, esp. since we are again aligned with the POSIX scheme?

@CxRes the argument I'd make for keeping the stricter flag, is that there'd be less userland breakage (and platforms like .NET and Deno have the recursive option, just in a stricter form).

The balance is between breaking existing users, vs., best possible design. I'm in the camp that, given there's prior art on other platforms of the stricter form of rmdir/recursive, perhaps we should aim to break less users in this case. The decision we make here, mainly effects where we would choose to output the deprecation warning.

Edit: @CxRes I have to admit too, I now find myself getting excited about ideas like an interactive rm 😆

Left a documentation related nit (it might be good to mention force only suppresses some types of errors).

Other than that, I'd just like to get a few more opinions on where we've chosen to include the deprecation warning -- make sure other folks are 👍 to keeping the strict form of recursive on rmdir.

CC: @nodejs/fs , @nodejs/tsc, @nodejs/tooling for review (making sure this is in the spirit of our conversation.)

Edit: @CxRes I have to admit too, I now find myself getting excited about ideas like an interactive rm 😆

@bcoe Let this land... I'll take you up on it (and other things rm)!!!!!

I believe tests should start passing once you merge these suggestions.

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

☝️ tests are green.

LGTM as what I believe we agreed to in the TSC discussions and since it looks like Matteo's suggestions have been addressed.

LGTM as what I believe we agreed to in the TSC discussions and since it looks like Matteo's suggestions have been addressed.

I would like to merge this later this afternoon ideally, given the tight timeline (unless folks come in with major concerns of course.).

lgtm

This PR introduces a new method fs.rm that provides the behaviour of rimraf when used with the recursive: true and force: true options. PR-URL: #35494 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Landed in 4e9f3cc

This PR introduces a new method fs.rm that provides the behaviour of rimraf when used with the recursive: true and force: true options. PR-URL: #35494 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) #35546 doc: * add aduh95 to collaborators (Antoine du Hamel) #35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512 PR-URL: TODO

This is a follow up to #35494 to add a deprecation warning when using recursive rmdir. This only warns if you are attempting to remove a file or a nonexistent path. PR-URL: #35562 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) #35546 doc: * add aduh95 to collaborators (Antoine du Hamel) #35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512 PR-URL: TODO

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) #35546 doc: * add aduh95 to collaborators (Antoine du Hamel) #35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) #35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) #35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) #35512 PR-URL: #35648

Hi there, sorry but I have a couple of questions about the maxRetries parameter:

Is it per file/directory?
If it is global, is the backoff reset on success?
If it is 0, does it mean infinite retries or no retries at all?
Why is it ignored if it's not recursive?

@rait there's also a conversation on this topic here (it's probably better to discuss on an open issue).

This option was carried over from rimraf.js, when rmdir/recursive was first implemented; the same option was carried over to rm.

@iansu @Trott it feels like, if nothing else, we need to better document the motivation of this retry, and explain its origin.

This PR introduces a new method fs.rm that provides the behaviour of rimraf when used with the recursive: true and force: true options. PR-URL: nodejs#35494 Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ruy Adorno <ruyadorno@github.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

This is a follow up to nodejs#35494 to add a deprecation warning when using recursive rmdir. This only warns if you are attempting to remove a file or a nonexistent path. PR-URL: nodejs#35562 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Ben Coe <bencoe@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com>

Has this been tested on windows ?

@GrosSacASac Yes, it has been tested on Windows. This functionality was previously in Node.js for about a year under the rmdir command. The underlying implementation is the same and comes from the rimraf package which has been around for years and is very widely used.

Notable changes: crypto: * update certdata to NSS 3.56 (Shelley Vohr) nodejs/node#35546 doc: * add aduh95 to collaborators (Antoine du Hamel) nodejs/node#35542 fs: * (SEMVER-MINOR) add rm method (Ian Sutherland) nodejs/node#35494 http: * (SEMVER-MINOR) allow passing array of key/val into writeHead (Robert Nagy) nodejs/node#35274 src: * (SEMVER-MINOR) expose v8::Isolate setup callbacks (Shelley Vohr) nodejs/node#35512 PR-URL: nodejs/node#35648

fs: add rm method

9845d05

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. labels Oct 4, 2020

iansu mentioned this pull request Oct 4, 2020

Make recursive rmdir more strict #35250

Closed

4 tasks

bcoe reviewed Oct 4, 2020

View reviewed changes

crypt096 approved these changes Oct 4, 2020

View reviewed changes

Trott reviewed Oct 4, 2020

View reviewed changes

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

Trott reviewed Oct 4, 2020

View reviewed changes

Comment thread doc/api/deprecations.md Outdated

Trott reviewed Oct 4, 2020

View reviewed changes

Comment thread doc/api/deprecations.md Outdated

Trott reviewed Oct 4, 2020

View reviewed changes

Comment thread doc/api/deprecations.md Outdated

aduh95 reviewed Oct 4, 2020

View reviewed changes

Comment thread doc/api/fs.md Outdated

Comment thread doc/api/deprecations.md Outdated

Comment thread doc/api/fs.md Outdated

code review changes

6c26d17

This comment has been minimized.

Sign in to view

bcoe approved these changes Oct 4, 2020

View reviewed changes

Comment thread doc/api/fs.md Outdated

bcoe added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Oct 4, 2020

Trott approved these changes Oct 5, 2020

View reviewed changes

ruyadorno approved these changes Oct 5, 2020

View reviewed changes

Comment thread lib/internal/errors.js Outdated

bengl reviewed Oct 5, 2020

View reviewed changes

Comment thread doc/api/fs.md Outdated

bcoe approved these changes Oct 7, 2020

View reviewed changes

Comment thread lib/fs.js Outdated

Comment thread lib/fs.js Outdated

bcoe approved these changes Oct 8, 2020

View reviewed changes

mhdawson approved these changes Oct 8, 2020

View reviewed changes

mcollina approved these changes Oct 8, 2020

View reviewed changes

bcoe closed this Oct 8, 2020

iansu mentioned this pull request Oct 8, 2020

fs: deprecation warning on recursive rmdir #35562

Closed

4 tasks

bcoe mentioned this pull request Oct 11, 2020

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

Closed

MylesBorins mentioned this pull request Oct 14, 2020

v14.14.0 proposal #35648

Merged

RyanZim mentioned this pull request Oct 17, 2020

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

Closed

hail2u added a commit to hail2u/hail2u.net that referenced this pull request Oct 23, 2020

Bookmark nodejs/node#35494

5ead933

bcoe mentioned this pull request Feb 6, 2021

fs: remove permissive rmdir recursive #37216

Merged

coderabbitai Bot mentioned this pull request Mar 24, 2026

feat: UI improvements, bug fixes, and protocol noise filtering 777genius/agent-teams-ai#25

Merged

4 tasks

This was referenced Jul 31, 2026

chore(workflow/impeccable): update Copilot skill to v4.0.4 nisavid/mastic#98

Merged

chore(scripts): dev-link for debugging skills from the checkout Viperwow/strict-ai#27

Merged

                 try {
-                  rimrafSync(tmpPath);
+                  rmSync(tmpPath);

+              async function rm(path, options) {
+                path = pathModule.toNamespacedPath(getValidatedPath(path));
+                options = validateRmOptionsSync(path, options);

fs: add rm method by iansu · Pull Request #35494 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: add rm method - #35494

fs: add rm method#35494iansu wants to merge 9 commits intonodejs:masternodejs/node:masterfrom iansu:fs-rmiansu/node:fs-rmCopy head branch name to clipboard

Conversation

iansu commented Oct 4, 2020

Checklist

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bcoe Oct 4, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bcoe Oct 4, 2020

Choose a reason for hiding this comment

Uh oh!

ljharb Oct 4, 2020

Choose a reason for hiding this comment

Uh oh!

bcoe Oct 5, 2020

Choose a reason for hiding this comment

Uh oh!

iansu Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

ljharb Oct 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

CxRes commented Oct 4, 2020

Uh oh!

bcoe commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CxRes commented Oct 4, 2020

Uh oh!

iansu commented Oct 4, 2020

Uh oh!

CxRes commented Oct 4, 2020

Uh oh!

bcoe commented Oct 4, 2020

Uh oh!

CxRes commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bcoe commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

bcoe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

bcoe commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CxRes commented Oct 4, 2020

Uh oh!

Uh oh!

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Oct 8, 2020 • edited by bcoe Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fs: add rm method#35494
iansu wants to merge 9 commits into
nodejs:masterfrom
iansu:fs-rm

bcoe commented Oct 4, 2020 •
edited
Loading

CxRes commented Oct 4, 2020 •
edited
Loading

bcoe commented Oct 4, 2020 •
edited
Loading

bcoe left a comment •
edited
Loading

bcoe commented Oct 4, 2020 •
edited
Loading

nodejs-github-bot commented Oct 8, 2020 •
edited by bcoe
Loading

ralt commented Oct 19, 2020 •
edited
Loading