doc: add err param to fs.copyFile callback

[F3n67u]

Fix #53230

Related issue: #52916

[RedYetiDev]

I believe fs.cp also needs this treatment

[LiviaMedeiros]

LGTM as is; however all related changes that should be made: returned value for fs.cpSync(), fs.copyFileSync(); and values passed to callback for fs.cp(), fs.copyFile().

Note that there is undefined explicitly passed as second parameter to fs.cp()'s callback on success: this happens, because cpFn() returns promise resolving with undefined and callback-based cp() is made using util.callbackify(). If this was unintentional, I guess it might be better to change it before documenting (especially since these API are marked as stable now...)

[F3n67u]

LGTM as is; however all related changes that should be made: returned value for fs.cpSync(), fs.copyFileSync(); and values passed to callback for fs.cp(), fs.copyFile().

Note that there is undefined explicitly passed as second parameter to fs.cp()'s callback on success: this happens, because cpFn() returns promise resolving with undefined and callback-based cp() is made using util.callbackify(). If this was unintentional, I guess it might be better to change it before documenting (especially since these API are marked as stable now...)

I have two questions:

1. Why we need to make changes to returned value for fs.cpSync() and fs.copyFileSync()? They both have no return value. Do you mean we have to document they return nothing(void)?

2. I just checked utils.callbackify(), err would be null instead of undefined when success. It seems the undefined value issue is not a problem?

node/lib/util.js

Lines 188 to 190 in 88d0701

	ReflectApply(original, this, args)
	.then((ret) => process.nextTick(cb, null, ret),
	(rej) => process.nextTick(callbackifyOnRejected, rej, cb));

[LiviaMedeiros]

• Why we need to make changes to returned value for fs.cpSync() and fs.copyFileSync()? They both have no return value. Do you mean we have to document they return nothing(void)?

Yes, they return undefined and we should make this explicit in the docs. Doesn't necessarily have to be done in this PR, my personal preference would be aligning whole file to * Returns: {something} syntax and removing redundant "Returns undefined"s in function descriptions.

• I just checked utils.callbackify(), err would be null instead of undefined when success. It seems the undefined value issue is not a problem?

err is the first parameter which is indeed null on success. When fs.cp() succeeds, it also passes undefined explicitly as second parameter to the callback. i.e. fs.cp(src, dst, (...params) => { console.log(params.length); }); would print 1 on error and 2 on success.

[F3n67u]

err is the first parameter which is indeed null on success. When fs.cp() succeeds, it also passes undefined explicitly as second parameter to the callback. i.e. fs.cp(src, dst, (...params) => { console.log(params.length); }); would print 1 on error and 2 on success.

Sorry. I misunderstood you. If that's the case, can we consider to fix it in another PR if it's necessary?

FYI, I added err to fs.cp()'s callback as well.

[LiviaMedeiros]

Sorry. I misunderstood you. If that's the case, can we consider to fix it in another PR if it's necessary?

Yes, of course.

[nodejs-github-bot]

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

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/53234
✔  Done loading data for nodejs/node/pull/53234
----------------------------------- PR info ------------------------------------
Title      doc: add `err` param to fs.copyFile callback (#53234)
Author     Feng Yu  (@F3n67u)
Branch     F3n67u:docs-copyfile-err -> nodejs:main
Labels     doc, fs, needs-ci
Commits    2
 - doc: add `err` param to fs.copyFile callback
 - doc: add err param to fs.cp callback
Committers 1
 - Feng Yu 
PR-URL: https://github.com/nodejs/node/pull/53234
Reviewed-By: LiviaMedeiros 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53234
Reviewed-By: LiviaMedeiros 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 31 May 2024 19:42:40 GMT
   ✔  Approvals: 2
   ✔  - LiviaMedeiros (@LiviaMedeiros): https://github.com/nodejs/node/pull/53234#pullrequestreview-2091640423
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/53234#pullrequestreview-2092020833
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-06-03T11:00:38Z: https://ci.nodejs.org/job/node-test-pull-request/59633/
- Querying data for job/node-test-pull-request/59633/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 53234
From https://github.com/nodejs/node
 * branch                  refs/pull/53234/merge -> FETCH_HEAD
✔  Fetched commits as 58711c2f8dbb..da77ab798ffd
--------------------------------------------------------------------------------
[main 296e9c7027] doc: add `err` param to fs.copyFile callback
 Author: Feng Yu 
 Date: Fri May 31 12:14:26 2024 -0700
 2 files changed, 2 insertions(+), 1 deletion(-)
[main a7bf82a713] doc: add err param to fs.cp callback
 Author: Feng Yu 
 Date: Fri May 31 14:45:18 2024 -0700
 2 files changed, 2 insertions(+), 1 deletion(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

doc: add err param to fs.copyFile callback
PR-URL: #53234

Reviewed-By: LiviaMedeiros [email protected]

Reviewed-By: Luigi Pinca [email protected]
[detached HEAD 391e1618e8] doc: add err param to fs.copyFile callback

Author: Feng Yu [email protected]

Date: Fri May 31 12:14:26 2024 -0700

2 files changed, 2 insertions(+), 1 deletion(-)

Rebasing (3/4)

Rebasing (4/4)
Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

doc: add err param to fs.cp callback
PR-URL: #53234

Reviewed-By: LiviaMedeiros [email protected]

Reviewed-By: Luigi Pinca [email protected]
[detached HEAD f808a8dfaa] doc: add err param to fs.cp callback

Author: Feng Yu [email protected]

Date: Fri May 31 14:45:18 2024 -0700

2 files changed, 2 insertions(+), 1 deletion(-)
Successfully rebased and updated refs/heads/main.
ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.

https://github.com/nodejs/node/actions/runs/9354148429

[nodejs-github-bot]

Landed in 21f38f8...eae75fe