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 recursive cp method #39372

Closed
wants to merge 1 commit into from
Closed

fs: add recursive cp method #39372

wants to merge 1 commit into from

Conversation

@bcoe
Copy link
Member

@bcoe bcoe commented Jul 13, 2021

Introduces recursive cp method, based on fs-extra implementation

Refs: nodejs/tooling#98
Fixes: #35880


Opening to start conversation.

TODO:

  • still have many tests to write, to increase coverage. (coverage is at 92%).
  • need to document sync and promise API.

CC: @nodejs/tooling, @jprichardson, @manidlou, @RyanZim

test/parallel/test-copy.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
@nodejs-github-bot

This comment has been hidden.

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy-sync.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina left a comment

lgtm

Copy link
Member

@targos targos left a comment

Just making sure this doesn't land without documentation for the Promises and sync APIs

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

This comment has been hidden.

@nodejs-github-bot

This comment has been hidden.

Copy link
Member

@iansu iansu left a comment

Looks good overall! 🙌

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
lib/internal/fs/utils.js Outdated Show resolved Hide resolved
@bcoe
Copy link
Member Author

@bcoe bcoe commented Jul 15, 2021

@mcollina @targos small, but significant update, I thought force would be a better option than overwrite for when clobbering the destination folder, and have renamed it to this. Also, force now defaults to false.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jul 15, 2021

lib/internal/fs/copy/copy-sync.js Outdated Show resolved Hide resolved
@RyanZim
Copy link

@RyanZim RyanZim commented Jul 15, 2021

I thought force would be a better option than overwrite for when clobbering the destination folder, and have renamed it to this. Also, force now defaults to false.

While we're rethinking options, should errorOnExist default to false or true? This is another one of those options that has its value for compat reasons.

@RyanZim
Copy link

@RyanZim RyanZim commented Jul 15, 2021

Also, for reference; discussion thread on how fs-extra should handle the forthcoming naming conflict: jprichardson/node-fs-extra#912

Copy link
Contributor

@aduh95 aduh95 left a comment

Further simplifications on the subject of replacing open + futimes + close with utimes.

lib/internal/fs/copy/copy-sync.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy-sync.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
@bcoe
Copy link
Member Author

@bcoe bcoe commented Jul 15, 2021

@RyanZim @iansu

While we're rethinking options, should errorOnExist default to false or true? This is another one of those options that has its value for compat reasons.

I was tempted to go with false, which I believe would make copying to the same folder twice relatively safe, i.e., you don't clobber files, but it's less of a pain in the neck to recover from a partial copy (you don't have to lookup an option).

Thoughts?

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

This comment has been hidden.

doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/errors.md Outdated Show resolved Hide resolved
doc/api/fs.md Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy-sync.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
lib/internal/fs/copy/copy.js Outdated Show resolved Hide resolved
@bcoe
Copy link
Member Author

@bcoe bcoe commented Jul 22, 2021

FYI, waiting on a windows machine at work (there are some hiccups with provisioning). Would happily pair with someone on fixing up the failing windows test, if anyone has a system up and running.

@targos
Copy link
Member

@targos targos commented Aug 5, 2021

@nodejs-github-bot

This comment has been hidden.

test/parallel/test-fs-cp.mjs Outdated Show resolved Hide resolved
test/parallel/test-fs-cp.mjs Outdated Show resolved Hide resolved
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 6, 2021

jasnell
jasnell approved these changes Aug 6, 2021
Copy link
Member

@jasnell jasnell left a comment

LGTM. Let's make sure we follow up on the path-as-buffer issues, however.

lib/internal/fs/promises.js Outdated Show resolved Hide resolved
iansu
iansu approved these changes Aug 6, 2021
joesepi
joesepi approved these changes Aug 6, 2021
Introduces recursive cp method, based on fs-extra implementation.
@nodejs-github-bot

This comment has been hidden.

aduh95
aduh95 approved these changes Aug 8, 2021
if (isPromise(shouldCopy)) {
throw new ERR_INVALID_RETURN_VALUE('boolean', 'filter', shouldCopy);
Copy link
Contributor

@aduh95 aduh95 Aug 8, 2021

Non blocking reflection, but probably something worth discussing before calling the API stable:
I'm not sure checking isPromise is the right thing to do here, %Array.prototype.filter% does not handle promises differently (JSON.stringify([0, 1].filter(async()=>false)) === '[0,1]'). Maybe we could emit a warning instead of throwing? That may help users spot if they have passed by mistake an async function, but wouldn't crash if they just happen to return a Promise object here.

Copy link
Member

@targos targos Aug 9, 2021

The reason that Array.prototype.filter doesn't handle promises differently is mostly historical. I can't think of a correct use case where the user wants to return a Promise and expects it to be treated as a truthy value.

Copy link
Member

@mcollina mcollina left a comment

lgtm

@nodejs-github-bot

This comment has been hidden.

@nodejs-github-bot

This comment has been hidden.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Aug 11, 2021

bcoe added a commit 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 <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ian Sutherland <ian@iansutherland.ca>
@bcoe
Copy link
Member Author

@bcoe bcoe commented Aug 12, 2021

Landed in 87d6fd7

@bcoe bcoe closed this Aug 12, 2021
danielleadams added a commit 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 <matteo.collina@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ian Sutherland <ian@iansutherland.ca>
danielleadams added a commit that referenced this issue Aug 16, 2021
Notable changes:

* fs:
  * add recursive cp method (Benjamin Coe) #39372
danielleadams added a commit that referenced this issue Aug 16, 2021
Notable changes:

* fs:
  * add recursive cp method (Benjamin Coe) #39372

PR-URL: #39782
danielleadams added a commit that referenced this issue Aug 17, 2021
Notable changes:

* fs:
  * experimental: add recursive cp method (Benjamin Coe) #39372

PR-URL: #39782
danielleadams added a commit that referenced this issue Aug 18, 2021
Notable changes:

* fs:
  * experimental: add recursive cp method (Benjamin Coe) #39372

PR-URL: #39782
@lostpebble
Copy link

@lostpebble lostpebble commented Aug 19, 2021

Is there any place to give feedback on this function?

Just looking through the docs, I think the option force: <boolean> is a bit ambiguous and would have been better suited to be named overwrite- as that's literally what it achieves, according to the docs.

[EDIT]
Sorry, after reading further comments, I see it was actually changed from overwrite to force - which I'm a little confused about now...

@bcoe
Copy link
Member Author

@bcoe bcoe commented Aug 26, 2021

@lostpebble please feel free to open an issue. As much as possible I'd like to model after cp's flags, but perhaps clobber is the better name in this case?

One good thing about force is it's consistent with rm.

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