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 rm method #35494

Closed
wants to merge 9 commits into from
Closed

fs: add rm method #35494

wants to merge 9 commits into from

Conversation

iansu
Copy link
Contributor

@iansu iansu commented Oct 4, 2020

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

@nodejs-github-bot 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 iansu mentioned this pull request Oct 4, 2020
4 tasks
Copy link
Member

@bcoe bcoe left a comment

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

test/pummel/test-policy-integrity.js Show resolved Hide resolved
test/parallel/test-fs-rmdir-recursive.js Outdated Show resolved Hide resolved
@@ -37,7 +37,7 @@ function onexit() {
process.chdir(testRoot);

try {
rimrafSync(tmpPath);
rmSync(tmpPath);
Copy link
Member

@bcoe bcoe Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love it 😄

lib/internal/fs/utils.js Show resolved Hide resolved
@@ -417,6 +418,13 @@ async function ftruncate(handle, len = 0) {
return binding.ftruncate(handle.fd, len, kUsePromises);
}

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

@bcoe bcoe Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Member

@ljharb ljharb Oct 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bcoe bcoe Oct 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@iansu iansu Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ljharb ljharb Oct 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

lib/internal/fs/utils.js Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
doc/api/deprecations.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
@CxRes
Copy link

CxRes commented Oct 4, 2020

Two points:

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

@bcoe
Copy link
Member

bcoe commented Oct 4, 2020

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.

@CxRes
Copy link

CxRes commented Oct 4, 2020

@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.

@iansu
Copy link
Contributor Author

iansu commented Oct 4, 2020

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

@CxRes
Copy link

CxRes commented Oct 4, 2020

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

@bcoe
Copy link
Member

bcoe commented Oct 4, 2020

@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.

@CxRes
Copy link

CxRes commented Oct 4, 2020

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!!!

@bcoe
Copy link
Member

bcoe commented Oct 4, 2020

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 😆

@nodejs-github-bot

This comment has been minimized.

bcoe
bcoe approved these changes Oct 4, 2020
Copy link
Member

@bcoe bcoe left a comment

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.

doc/api/fs.md Outdated Show resolved Hide resolved
@bcoe
Copy link
Member

bcoe commented Oct 4, 2020

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

@bcoe 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
@CxRes
Copy link

CxRes commented Oct 4, 2020

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)!!!!!

Trott
Trott approved these changes Oct 5, 2020
lib/internal/errors.js Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
bcoe
bcoe approved these changes Oct 7, 2020
Copy link
Member

@bcoe bcoe left a comment

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

lib/fs.js Outdated Show resolved Hide resolved
lib/fs.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

bcoe
bcoe approved these changes Oct 8, 2020
@nodejs-github-bot
Copy link
Contributor

nodejs-github-bot commented Oct 8, 2020

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

☝️ tests are green.

Copy link
Member

@mhdawson mhdawson left a comment

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

@bcoe
Copy link
Member

bcoe commented Oct 8, 2020

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.).

Copy link
Member

@mcollina mcollina left a comment

lgtm

bcoe pushed a commit that referenced this pull request Oct 8, 2020
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>
@bcoe
Copy link
Member

bcoe commented Oct 8, 2020

Landed in 4e9f3cc

@bcoe bcoe closed this Oct 8, 2020
BethGriggs pushed a commit that referenced this pull request Oct 13, 2020
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>
MylesBorins added a commit that referenced this pull request Oct 14, 2020
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
@MylesBorins MylesBorins mentioned this pull request Oct 14, 2020
bcoe pushed a commit that referenced this pull request Oct 15, 2020
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>
MylesBorins added a commit that referenced this pull request Oct 15, 2020
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
MylesBorins added a commit that referenced this pull request Oct 15, 2020
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
MylesBorins added a commit that referenced this pull request Oct 16, 2020
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
@ralt
Copy link
Contributor

ralt commented Oct 19, 2020

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?

@bcoe
Copy link
Member

bcoe commented Oct 20, 2020

@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.

hail2u added a commit to hail2u/hail2u.net that referenced this pull request Oct 23, 2020
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
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>
@GrosSacASac
Copy link
Contributor

GrosSacASac commented Jan 18, 2021

Has this been tested on windows ?

@iansu
Copy link
Contributor Author

iansu commented Jan 18, 2021

@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.

ryanhc pushed a commit to Samsung/lwnode that referenced this pull request Jun 29, 2022
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet