doc: add callback function signatures in fs.md

[matejkrajcovic]

This is related to #11135.

Adds descriptions of callback parameters to function signatures as in child_process docs.

Checklist

• documentation is changed or added
• commit message follows commit guidelines

Affected core subsystem(s)

doc, fs

[mscdex]

I'm all for clearly documenting this but I'm not sure this is the best way to communicate the signature and/or possible values. For example, the error parameter may contain a false-y value and not an actual Error instance (so seeing the Error type for this parameter may throw new users off), or the error parameter may contain an Error instance and the other parameters either may not exist at all or they may be set to undefined. In the latter scenario this may not matter much in practice, it just depends on how exact we want to be in our documentation (e.g. in case someone is checking arguments.length in their callbacks).

[jasnell]

I have to agree with @mscdex. I love that you're taking the time to document these... it is definitely needed. But we should find a way of representing them that is easier to follow. Perhaps:

• callback {Function} callback(error, arg1, arg2)

Or something similar

[refack]

👍

[refack]

I'm all for clearly documenting this but I'm not sure this is the best way to communicate the signature and/or possible values. For example, the error parameter may contain a false-y value and not an actual Error instance (so seeing the Error type for this parameter may throw new users off), or the error parameter may contain an Error instance and the other parameters either may not exist at all or they may be set to undefined. In the latter scenario this may not matter much in practice, it just depends on how exact we want to be in our documentation (e.g. in case someone is checking arguments.length in their callbacks).

IMHO not perfect but better than nothing.

[matejkrajcovic]

• Can really error argument be just falsy and not an instance of Error? I have checked multiple functions and tried to understand C++ code in src/node_file.cc and it seems to me it is always an instance of Error.
• I've used this style of signatures to be consistent with child_process.fs and dns.fs. I can change it to callback {Function} callback(error, arg1, arg2) but then we also should change those files.

I prefer to leave callback arguments on separate lines. This seems clearer to me:

• callback {Function}
  • err {Error}
  • written {integer}
  • string {string}

than this:

• callback {Function} callback(err {Error}, written {integer}, string {string})

[mscdex]

Can really error argument be just falsy and not an instance of Error?

Yes, when no error occurred.

[matejkrajcovic]

I see. err is null when no error happens and arguments with results are undefined in case of error. Is this better?

• callback {Function}
  • err {Error|null}
  • written {integer|undefined}
  • string {string|undefined}

[tniessen]

I am sorry for the lack of feedback from our side.

• callback {Function}
  • err {Error|null}
  • written {integer|undefined}
  • string {string|undefined}

IMO it is not better, I think it is more confusing than without those additions. I am fine with Error|null, but someone might complain that it is inconsistent. Alternatively, we could add a note after err within the same line, we do that for other parameters as well, but it would be quite repetitive as we have a lot of callbacks with an error parameter.

What do you think @mscdex ?

[mscdex]

I agree that the currently proposed layout isn't ideal either. Also, the err argument may not always be null, it could be undefined as well for some parts of the API.

[BridgeAR]

I think the proposal itself is better then the current situation and therefore I am +1 for this.

I think it makes sense to make clear that the error can be null (or undefined in some APIs, but I guess that is rare?) but I also agree that adding |undefined all the time might be confusing. Maybe a mixture would be a good compromise? It is not ideal but I think it makes it somewhat clearer.

• callback {Function}
  • err {Error|null}
  • written {integer}
  • string {string}

Or we mark the other arguments as "optional" because they will only be returned in case err === null.

• callback {Function}
  • err {Error|null}
  • [written {integer}]
  • [string {string}]

[BridgeAR]

@matejkrajcovic this needs a rebase.

[matejkrajcovic]

@BridgeAR rebased.

[BridgeAR]

@mscdex are you fine with landing this as is? I think it is better than our current situation and we can still try to figure out how to improve this further at another point?

[mscdex]

Both of these should be plural.

[BridgeAR]

@matejkrajcovic seems like we are almost there and only one comment has to be addressed.

[matejkrajcovic]

@BridgeAR it's fixed.

[BridgeAR]

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

[BridgeAR]

Landed in a636b72

[MylesBorins]

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported