src,lib: rename FSReqWrap to FSReqCallback

[maclover7]

Given that FSReqPromise does not inherit from FSReqWrap, FSReqWrap
should be renamed FSReqCallback to better describe what it does.

First of a few upcoming fs refactorings :)

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[nodejs-github-bot]

@maclover7 build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/347/pipeline

[maclover7]

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

[joyeecheung]

The change to the binding and to the resource types are potentially breaking so this may be semver-major.

cc @nodejs/diagnostics Does it matter if the async resource type FSREQWRAP is now FSREQCALLBACK?

[mmarchini]

I understand the reason behind this, but doesn't that mean we'll need to change the name of every wrapper once its API get a promise interface? If that's the case, we might as well change everything at once to avoid multiple breaking changes (the changes to async_hooks are semver-major IMO).

[maclover7]

I understand the reason behind this, but doesn't that mean we'll need to change the name of every wrapper once its API get a promise interface?

@mmarchini Unfortunately, it depends. For example, the PR to add the promisified dns module did not require any changes to the C++ binding layer to support promises due to how the binding was originally implemented, but fs did. It seems like based on how things stand at the binding layer, this will have to be on a case-by-case basis.

[mcollina]

LGTM

[joyeecheung]

FWIW, putting WRAP, PROMISE or CALLBACK in the types already seem to be leaking too many implementation details, having downstream consumers depend on these details may not be a good idea in terms of maintainability.

[Flarna]

The doc in async hooks states "The type is a string identifying the type of resource that caused init to be called. Generally, it will correspond to the name of the resource's constructor.".

Based on this we have to leak implementation details. On the other hand there is no detailed specification which async resources exist (within node core) and what exactly they represent.

I would also prefer to have a cleaner separation of implementation details and async hook resource names.

As async hooks are still experimental such an API change should be currently not semver major to my understanding - but will be once they are no longer experimental.

[jasnell]

Not technically semver-major, no, but async_hooks are starting to be used extensively so we should still be careful and considerate about such changes.

[addaleax]

Uh … can we split this into a backportable part and a semver-major change for the async_hooks identifier? Otherwise this would be a wonderful source of merge conflicts…

[maclover7]

Updated @addaleax (note to self/future backporters -- the second commit is the semver-major one :))

[maclover7]

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

[maclover7]

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

[maclover7]

Landed in 27a5338...f479050, thank you for the reviews!