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

Recursive copy #98

Closed
iansu opened this issue Jan 8, 2021 · 4 comments
Closed

Recursive copy #98

iansu opened this issue Jan 8, 2021 · 4 comments

Comments

@iansu
Copy link
Contributor

iansu commented Jan 8, 2021

Implement a recursive copy command in Node.js. I have been drafting a proposal for this functionality in this document: https://hackmd.io/Q2O7hVyZSKClYW9Vyipeig

At this stage I'd like feedback from the Tooling group on the proposal. Once we're happy with it we can open an issue in the main Node.js repo to get broader feedback. If all goes well we can follow up with an implementation.

@iansu
Copy link
Contributor Author

iansu commented Feb 19, 2021

@RyanZim We're considering using the fs-extra recursive copy implementation in Node.js. I'd love your feedback on this proposal and on incorporating part of fs-extra into Node core. cc @CxRes

@RyanZim
Copy link

RyanZim commented Feb 20, 2021

@manidlou is the collaborator that handles most things related to copy and move in fs-extra; so looping him in here since his feedback is probably most useful here.

Would you be planning on implementing the filter option as well?

@CxRes
Copy link

CxRes commented Feb 20, 2021

I am not involved with fs-extra though I use it in my work. And, I find the copy function incredibly useful.

However, from the point of view of nodejs, I think the copy function as implemented in fs-extra is limited. Don't get me wrong, it is really good for day to day use, its just that recursive copy is a relatively complex operation.

I would suggest that you consider cp, rsync and Windows Robocopy for inspiration.

A few things, I would like are:

  • A verbose output as a return value
  • A filter that can take regex and/or glob array (most times I end up importing picomatch)

Perhaps later you can add:

  • Preserve attributes, permissions and time-stamps
  • interactive mode

@manidlou
Copy link

manidlou commented Jul 10, 2021

@iansu I just want to remind you that our copy implementation in fs-extra is a bit different than unix cp command.

For instance, in fs-extra, when copying a file to a different directory, like copy('src-file.js', 'some/dir/dest-file.js') the user requires to explicitly specify the destination file name meaning copy('src-file.js', 'some/dir/') would fail in fs-extra while it works fine in the unix cp command.

That was a source of confusion for some of our users because they have this assumption that fs-extra functions would work like unix commands (which is a fair assumption since node is very unix-y) but we at fs-extra decided to go with explicitness.

So probably something that you might want to consider when deciding on the behavior of the function.

bcoe added a commit to bcoe/node-1 that referenced this issue Jul 13, 2021
Introduces recursive copy method, based on fs-extra implementation

Refs: nodejs/tooling#98
bcoe added a commit to bcoe/node-1 that referenced this issue Jul 13, 2021
Introduces recursive copy method, based on fs-extra implementation

Refs: nodejs/tooling#98
bcoe added a commit to bcoe/node-1 that referenced this issue Jul 25, 2021
Introduces recursive copy method, based on fs-extra implementation

Refs: nodejs/tooling#98
bcoe added a commit to bcoe/node-1 that referenced this issue Jul 26, 2021
Introduces recursive copy method, based on fs-extra implementation.
See: nodejs/tooling#98
bcoe added a commit to bcoe/node-1 that referenced this issue Aug 4, 2021
Introduces recursive copy method, based on fs-extra implementation.
See: nodejs/tooling#98
bcoe added a commit to nodejs/node that referenced this issue Aug 12, 2021
Introduces recursive cp method, based on fs-extra implementation.

PR-URL: #39372
Fixes: #35880
Refs: nodejs/tooling#98
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ian Sutherland <[email protected]>
danielleadams pushed a commit to nodejs/node that referenced this issue Aug 16, 2021
Introduces recursive cp method, based on fs-extra implementation.

PR-URL: #39372
Fixes: #35880
Refs: nodejs/tooling#98
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Antoine du Hamel <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ian Sutherland <[email protected]>
@bcoe bcoe closed this as completed Jun 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants