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?

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

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

+1 to going stable with it.

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

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.

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

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.

• 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

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?

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

@nodejs/fs

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.

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

• .NET: https://docs.microsoft.com/en-us/dotnet/api/system.io.directory.delete?view=netcore-3.1:
  • my colleagues believe system.io.directory.delete also throws if given a file path (have not confirmed).
• Deno: https://doc.deno.land/builtin/stable#Deno.remove
  • throws on unfound directory Uncaught NotFound: No such file or directory.
  • deletes both directories and files.
• Python's shutil.rmtree throws on file: NotADirectoryError: [Errno 20]
• Ruby's FileUtils.remove_dir follows the behavior of rm -rf, it does not throw.

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

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.

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

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.

Note: this will be breaking, as @Trott points out, and should be clearly called out as a notable change in the major its released in.

Changes to experimental features aren't semver-major, so I guess it depends how we go about this....

@Trott @BridgeAR @mcollina @mhdawson, etc., please take a look based on conversation today:

1. removes experimental language from rmdir/recursive.
2. notes that the "loose" behavior is deprecated.

@iansu has offered to update their work, to add the new rm method, at which point I figure we can add a deprecation warning when the recursive option is used.

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

I plan on landing this tomorrow hopefully, please let me know if anyone has additional feedback.

Landed in 35b17d9