-
Notifications
You must be signed in to change notification settings - Fork 30k
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
net,child_process: improve naming in internal code #14449
Conversation
All of this code is internal-only, and the changed variables/methods are not generally useful to userland code. When backporting this to release branches, it might be appropriate to add non-enumerable aliases to be 100 % sure.
While I'm definitely +1 on the change, there's a non-zero chance that this will break existing code. While we shouldn't be too sympathetic to users access undocumented private properties, we know they do so. The old property names should go through a proper deprecation cycle and be recast as aliases to the new names, at least until Node.js 10.0.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is green. CITGM run for good measure would be a good idea, although I guess we do that for all semver-major PRs now?
Ooof, yeah, deprecation cycle as mentioned by @jasnell...Totally +1 to moving in this direction though, let's do it!
@Trott This is already getting a CITGM run. :) CI is green modulo unrelated failures, I don’t see anything concerning in the CITGM output. @jasnell I know you like deprecations but it’s already pretty unrealistic that userland code touches these methods to begin with. They are not conveying information that would generally be useful to the outside world. If you are 100 % sure that’s the best idea, I can add deprecated aliases here, but I think it’s much more likely to be unnecessary overhead (and re-introducing inacceptable language into our documentation) if we do so. |
We've been bitten before when making assumptions about how unlikely it is that someone is using a private API. Making this change is the right thing to do, but so is protecting existing users. We should do both. |
@jasnell done, PTAL |
@@ -634,6 +634,17 @@ Type: Runtime | |||
|
|||
*Note*: change was made while `async_hooks` was an experimental API. | |||
|
|||
<a id="DEP00XX"></a> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I see from Git history, deprecations are usually assigned a number immediately in the pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t care much but I think @jasnell has been telling people multiple times to assign the number while landing :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, it's better to assign when landing... or likely better yet to handle it like the REPLACEME
metadata eventually. The reason is because if we end up with more than one deprecation PR at any given time, there's a greater chance of accidentally duplicating the codes or getting them out of order somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for clarifying :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @addaleax. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM if CI is green
Landed in 75a19fb |
All of this code is internal-only, and the changed variables/methods are not generally useful to userland code. When backporting this to release branches, it might be appropriate to add non-enumerable aliases to be 100 % sure. PR-URL: #14449 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Missed while landing 75a19fb. Ref: nodejs#14449
Remove the getters introduced in 75a19fb. Refs: nodejs#14449
Remove the getters introduced in 75a19fb. PR-URL: #17141 Refs: #14449 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
[trigger warning for use of racist language in the actual diff]
All of this code is internal-only, and the changed variables/methods are not generally useful to userland code.
When backporting this to release branches, it might be appropriate to add non-enumerable aliases to be 100 % sure.
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
net, child_process