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: remove experimental language from rmdir recursive #35171

Closed
wants to merge 1 commit into from

Conversation

bcoe
Copy link
Contributor

@bcoe bcoe commented Sep 13, 2020

I would like to make a case for removing the experimental flag from rmdir's recursive option:

  1. the feature landed over a year ago.
  2. the functionality is quite useful to tooling authors (we use it in Node.js' own test suite, see: test: work around ENOTEMPTY when cleaning tmp dir #30849),
  3. users are hesitant to use the recursive option, due to it being experimental.
  4. I feel that this feature aligns closely with the value of "Developer Experience", described in the technical values document @mhdawson's is working on.

Regarding #34278, "Rethink recursive flag".

As a compromise, I initially looked at raising an ENOENT exception, if the initial path provided to rmdir did not exist ... I was thinking perhaps we could get away with deviating a bit from rimraf's behavior, in the name of reaching consensus.

This immediately broke our test test suite, because we rely on the current recursive behavior in our tmpdir.js helper ... It also drilled home for me that deviating from rimraf might not be a great idea...

The current implementation of rmdir + recursive provides behavior the community is accustomed to, due to its similarity to rimraf.

An alternative compromise

What if we explicitly call out in documentation the fact that setting recursive to true gives you behavior that matches the community module rimraf?

It might be the case that, due to some of the oddities in this API surface, a module like fs-extra avoids setting the recursive option. But, as our own use case in tmdir.js demonstrates, the recursive option, as it exists today, is great for a variety of tooling needs.

CC: @CxRes, @nodejs/tooling, @RyanZim, @iansu

Refs: #34278

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 doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. labels Sep 13, 2020
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

The Promise-based rmdir() should be updated as well.

doc/api/fs.md Outdated
@@ -3532,6 +3530,10 @@ to the completion callback.
Using `fs.rmdir()` on a file (not a directory) results in an `ENOENT` error on
Windows and an `ENOTDIR` error on POSIX.

Setting `recursive` to `true` results in behavior similar to the rimraf package
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be OK to drop the reference to rimraf and just explain this function's behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My concern here is that rimraf implies a lot of behavior, perhaps we can work together and figure out the most important behavioral things to call out? I chose two things that stood out for me, from what @RyanZim and @CxRes mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

My personal point of view: I don't think I've ever directly used the rimraf module in my own code, so referencing it here would force me to go read about implied behavior of an npm module in order to understand how core works. That feels a little backward.

I don't feel strongly about it though. If you think it's important to note, then it's fine with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cjihrig I looked at the rimraf docs, they point out that rimraf has behavior similar to rm -rf. Why don't we use this language instead of calling out the module name?

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds good to me 👍

@bcoe
Copy link
Contributor Author

bcoe commented Sep 13, 2020

Playing a bit with GitHub's search, it's also clear that quite a few folks have been using the recursive option.

This perhaps speaks to the fact that we should have had a warning when the recursive setting was added? It feels like, going forward, we should do this for experimental options?

@cjihrig
Copy link
Contributor

cjihrig commented Sep 13, 2020

It feels like, going forward, we should do this for experimental options?

I believe that is already the policy, it was just missed here.

@Trott
Copy link
Member

Trott commented Sep 13, 2020

+1 to going stable with it.

Maybe the ENOENT behavior can be made available with an option? (Not in this PR, though.)

@bcoe
Copy link
Contributor Author

bcoe commented Sep 13, 2020

Maybe the ENOENT behavior can be made available with an option? (Not in this PR, though.)

@Trott I really like this idea, perhaps we introduce an option like recursiveStrict and work with the fs-extra to make the behavior closer to what folks would expect building a library on top of it.

@gengjiawen gengjiawen added notable-change PRs with changes that should be highlighted in changelogs. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 14, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 14, 2020
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@CxRes
Copy link

CxRes commented Sep 14, 2020

Although I can sympathize with the intent of this PR, it seems like a band-aid to cover up what is already an oversight of design. The problem of going ahead with just the language change is that deeper issues that have been raised, will in all likelihood remain unaddressed, which I think is the worst possible outcome.

At least, I would urge the community to make a good faith attempt to find solutions from which we might find the compromise. I, for example, would not mind the introduction of new flags as a compromise, as suggested above (though my preference would be to deprecate the recursive flag but not removing it for the foreseeable future).

Also, I agree that the documentation should refer to rm -rf and not rimraf.

@bcoe It seems you are trying to argue your case both ways:

users are hesitant to use the recursive option, due to it being experimental.

and

Playing a bit with GitHub's search, it's also clear that quite a few folks have been using the recursive option.

I point this out, not to criticize, but to bring to attention to the deeper questions on how new features are to be onboarded in Node! These are more social issues such as what should be honored - documentation or popularity. Should we change experimental feature to achieve a more consistent API or will it be the case that experimental features will not be changed once they become reasonably popular (or when warnings have not been added)? This question is above my pay grade (though I hold strong opinions); what is certain is that either answer will lead to considerable developer pain.

@boneskull
Copy link
Contributor

  • To be clear: I am -1 on deprecating any part of the recursive flag in any manner
  • The current behavior is useful and what most consumers want.
  • While it works for most, the recursive flag does not work for all, as noted
  • Removing the "experimental" status is not mutually exclusive with adding an option for changing ENOENT behavior

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@RyanZim
Copy link
Contributor

RyanZim commented Sep 14, 2020

Removing the "experimental" status is not mutually exclusive with adding an option for changing ENOENT behavior

@boneskull to be clear, are you flatly opposed to changing the way that recursive works by default in any way, including fixing the fact that it deletes both files and directories?

@boneskull
Copy link
Contributor

It should not remove a file if given a file (unsure if that's still a bug). But it should remove all files within a directory if given a directory.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 15, 2020

@bcoe It seems you are trying to argue your case both ways:

There are two different groups of people 😛

  • many users are using the functionality as it is implemented, and we would break them with significant changes to the API.
  • the fact that the feature is experimental limits another large group of users from adopting it.

At least, I would urge the community to make a good faith attempt to find solutions from which we might find the compromise. I, for example, would not mind the introduction of new flags as a compromise, as suggested above (though my preference would be to deprecate the recursive flag but not removing it for the foreseeable future).

I am an advocate for an additional flag (perhaps recursiveStrict?). We could work with you to make sure is closer to the expectations from a POSIX perspective.

I am also -1 on deprecating the existing feature. Wearing the hat of someone writing small command line tools, behavior like rm -rf is frequently what I want.

I point this out, not to criticize, but to bring to attention to the deeper questions on how new features are to be onboarded in Node!

I care deeply about listening to people and finding great compromises whenever possible. I apologize that this wasn't how I came across in my initial reaction to #34278.

I think what's difficult here is that we have two significantly different perspectives:

  • quite a few folks in this thread are quite happy with behavior like rm -rf, they like the magic one liner.
  • you bring an alternate perspective, that you would rather the functionality is less magical, leaving more error handling and configuration to the user (but allowing tools built on the functionality to surface more nuance to the user).

@Trott
Copy link
Member

Trott commented Sep 15, 2020

@nodejs/fs

@RyanZim
Copy link
Contributor

RyanZim commented Sep 15, 2020

It should not remove a file if given a file (unsure if that's still a bug). But it should remove all files within a directory if given a directory.

As of Node v14.10.1, fs.rmdir(file, {recursive: true}) still deletes the file with no error. This should be fixed before this is considered stable.

As far as the ENOENT stuff goes, it's still a bit odd, but if it's clearly documented (which it currently is), I'm not going to make a big fight about it. In a perfect world, recursive would be its own method, but unfortunately, it looks like we're past that point.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 15, 2020

@RyanZim

As of Node v14.10.1, fs.rmdir(file, {recursive: true}) still deletes the file with no error. This should be fixed before this is considered stable.

My concern about changing this behavior is just that it moves us away from the behavior of rm -rf, and makes the feature slightly harder to describe/puts a bit more burden on the user.

I'm personally -0 on this change; but would be supportive of a consensus in either direction.

One thought, it might be good to look at some of the other platforms that have added rmdir recursive:

I'd be interested to see what decision these other platforms have taken, when given a file path rather than directory.

@RyanZim
Copy link
Contributor

RyanZim commented Sep 15, 2020

My concern about changing this behavior is just that it moves us away from the behavior of rm -rf, and makes the feature slightly harder to describe/puts a bit more burden on the user.

It does move away from rm -rf, but this is recursive rmdir, not rm. I don't see how this really makes it harder to describe, it's doing exactly what it says, recursively removing a directory, with a note that it won't throw if the path doesn't exist.

@bcoe
Copy link
Contributor Author

bcoe commented Sep 15, 2020

@RyanZim having done a bit of a review of other languages, I've honestly come around to either wanting us to stick with the very liberal implementation (how things sit today).

Or moving wholesale to a stricter implementation where both non directory paths and missing paths throw. I couldn't find a precedent that throws on a non-directory, but allows a missing path (in the languages I looked at).

@CxRes
Copy link

CxRes commented Sep 16, 2020

Let me propose a part of my original solution as alternative that might satisfy both the groups of users and cause minimum disruption (but at the cost of some redundancy):

Lets leave rmdir -r as it is (To @bcoe's point) except that it will throw when deleting a file only (To @RyanZim's point) and maybe on a missing path (I wouldn't bother too much either way due to what comes next). Lets then have another function rm (or you might want to call it something else) with a strict implementation matching (at least similar to) POSIX rm that gives users more fine-grained control.

@bcoe bcoe closed this Oct 3, 2020
@bcoe bcoe deleted the rmdir-recursive branch October 3, 2020 18:40
@nodejs nodejs deleted a comment Oct 3, 2020
@iansu iansu mentioned this pull request Oct 4, 2020
4 tasks
danielleadams pushed a commit that referenced this pull request Oct 6, 2020
Refs: #34278

PR-URL: #35171
Reviewed-By: Christopher Hiller <boneskull@boneskull.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@danielleadams danielleadams mentioned this pull request Oct 6, 2020
danielleadams added a commit that referenced this pull request Oct 6, 2020
Notable changes:

* fs:
  * remove experimental from rmdir recursive (Benjamin Coe) [#35171](#35171)

PR-URL: #35525
danielleadams added a commit that referenced this pull request Oct 7, 2020
Notable changes:

* fs:
  * remove experimental from rmdir recursive (Benjamin Coe) [#35171](#35171)

PR-URL: #35525
andersk added a commit to andersk/node that referenced this pull request Nov 16, 2020
This was missed in commit 35b17d9.

Refs: nodejs#34278
Refs: nodejs#35171

Signed-off-by: Anders Kaseorg <andersk@mit.edu>
@andersk
Copy link
Contributor

andersk commented Nov 16, 2020

This missed the Stability: 1 line under fsPromises.rmdir. Fix submitted as #36131.

nodejs-github-bot pushed a commit that referenced this pull request Nov 19, 2020
This was missed in commit 35b17d9.

Refs: #34278
Refs: #35171

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

PR-URL: #36131
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Nov 22, 2020
This was missed in commit 35b17d9.

Refs: #34278
Refs: #35171

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

PR-URL: #36131
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
This was missed in commit 35b17d9.

Refs: #34278
Refs: #35171

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

PR-URL: #36131
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
This was missed in commit 35b17d9.

Refs: #34278
Refs: #35171

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

PR-URL: #36131
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
This was missed in commit 35b17d9.

Refs: #34278
Refs: #35171

Signed-off-by: Anders Kaseorg <andersk@mit.edu>

PR-URL: #36131
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Refs: nodejs#34278

PR-URL: nodejs#35171
Reviewed-By: Christopher Hiller <boneskull@boneskull.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Notable changes:

* fs:
  * remove experimental from rmdir recursive (Benjamin Coe) [nodejs#35171](nodejs#35171)

PR-URL: nodejs#35525
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. doc Issues and PRs related to the documentations. fs Issues and PRs related to the fs subsystem / file system. notable-change PRs with changes that should be highlighted in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.