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

src,lib: rename FSReqWrap to FSReqCallback #21971

Closed
wants to merge 2 commits into from

Conversation

maclover7
Copy link
Contributor

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 nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 25, 2018
@maclover7 maclover7 added the fs Issues and PRs related to the fs subsystem / file system. label Jul 26, 2018
@maclover7
Copy link
Contributor Author

@joyeecheung
Copy link
Member

joyeecheung commented Jul 26, 2018

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
Copy link
Contributor

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
Copy link
Contributor Author

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 mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 26, 2018
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joyeecheung
Copy link
Member

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
Copy link
Member

Flarna commented Jul 26, 2018

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
Copy link
Member

jasnell commented Jul 27, 2018

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
Copy link
Member

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…

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 :)
@maclover7
Copy link
Contributor Author

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

@maclover7
Copy link
Contributor Author

@maclover7
Copy link
Contributor Author

@maclover7
Copy link
Contributor Author

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

@maclover7 maclover7 closed this Aug 1, 2018
@maclover7 maclover7 deleted the jm-fs-req branch August 1, 2018 19:07
maclover7 added a commit that referenced this pull request Aug 1, 2018
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 :)

PR-URL: #21971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
maclover7 added a commit that referenced this pull request Aug 1, 2018
PR-URL: #21971
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
pull bot pushed a commit to SimenB/node that referenced this pull request Nov 2, 2018
Correct async hooks resource names to match the implementation:
`FSREQWRAP` => `FSREQCALLBACK`
`TCPSERVER` => `TCPSERVERWRAP`

PR-URL: nodejs#24001
Refs: nodejs#21971
Refs: nodejs#17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this pull request Nov 3, 2018
Correct async hooks resource names to match the implementation:
`FSREQWRAP` => `FSREQCALLBACK`
`TCPSERVER` => `TCPSERVERWRAP`

PR-URL: #24001
Refs: #21971
Refs: #17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 21, 2018
Correct async hooks resource names to match the implementation:
`TCPSERVER` => `TCPSERVERWRAP`

Backport-PR-URL: #24683
PR-URL: #24001
Refs: #21971
Refs: #17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Dec 26, 2018
Correct async hooks resource names to match the implementation:
`TCPSERVER` => `TCPSERVERWRAP`

Backport-PR-URL: #24683
PR-URL: #24001
Refs: #21971
Refs: #17157
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants