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

Add options to fse.remove*() ? #840

Closed
RyanZim opened this issue Oct 17, 2020 · 9 comments
Closed

Add options to fse.remove*() ? #840

RyanZim opened this issue Oct 17, 2020 · 9 comments

Comments

@RyanZim
Copy link
Collaborator

RyanZim commented Oct 17, 2020

Opening a new issue to continue discussions from #246 & #785 on a fresh thread. CC: @CxRes @kutyepov

There have been various proposals to add an option that would make fse.remove error on nonexistent paths. Given that Node's new fs.rm method supports this (#806), I'm open to the idea. However, I would appreciate input from the other maintainers here.

Another remaining question is what this option should be named. force would follow fs.rm, but it would be a default-true option, which I kind of dislike. shoutMissing was proposed in #785, but I'm not strongly for or against that name. Naming stuff is hard, ideas wanted.

@manidlou
Copy link
Collaborator

Yeah since Node has that option, we should add that too. I also agree with you that we should use fs.rm wherever is possible.

Naming stuff is hard

😄 agreed! I am not sure about the name tho! Maybe errorIfNotExists?!

@CxRes
Copy link

CxRes commented Oct 26, 2020

Should we not align ourselves with fs.rm, effectively back porting it to earlier editions of node? Unless there is a strong reason for introducing a different behaviour?

@RyanZim
Copy link
Collaborator Author

RyanZim commented Oct 27, 2020

Our default behavior cannot change, and our default behavior is the exact opposite of fs.rm. fse.remove currently behaves like fs.rm with recursive, force.

@CxRes
Copy link

CxRes commented Oct 28, 2020

From what I recall, this default behavior was chosen because of the rmdir -r choice made by nodejs. In the light of present changes to node's api, I would think fse should ideally not continue with this kitchen sink choice. On the other hand, I understand that we do not want to break user-land. Perhaps, a new namespace like fse.delete?

@RyanZim
Copy link
Collaborator Author

RyanZim commented Oct 29, 2020

From what I recall, this default behavior was chosen because of the rmdir -r choice made by nodejs.

No, fse.remove has worked the way it does since the beginning of fs-extra, which far predates Node having any kind of recursive function. fse.remove cannot change its behavior.

The way I see it, we have 3 options:

  1. Add options to fse.remove to make it function more like fs.rm, but keep default behavior.
    • This has the advantage of being able to reuse existing code.
  2. Add a new method that's like fs.rm (i.e. your fse.delete proposal).
    • This requires duplicating code, and creates a method that will have to be removed in the future.
  3. Polyfill fs.rm for older versions of Node.
    • AFAIK, we've never done polyfilling of Node methods. This requires precision to match Node's behavior, but does have a strong argument for being the least disruptive to users of fs-extra.

@kutyepov
Copy link

kutyepov commented Nov 1, 2020

What if fse.remove is changed to error out when a file is not present by default? This will align it with node's rm function and won't introduce additional overhead in maintaining the library. I understand that this will be a breaking change, but that could be released as a major version of the library.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Nov 2, 2020

fse.remove has existed with its current behavior since the inception of this library AFAIK; changing default behavior is non-negotiable.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Oct 20, 2022

fs.rm is supported in all maintained LTS versions of Node, so closing this out. If you want fs.rm behavior, use fs.rm.

@RyanZim RyanZim closed this as completed Oct 20, 2022
@RyanZim
Copy link
Collaborator Author

RyanZim commented Oct 20, 2022

As per #968 (comment); fse.remove will become a thin wrapper around fs.rm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants