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

net,child_process: improve naming in internal code #14449

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

[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), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

net, child_process

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.
@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. net Issues and PRs related to the net subsystem. labels Jul 24, 2017
@addaleax
Copy link
Member Author

@jasnell
Copy link
Member

jasnell commented Jul 24, 2017

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

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 24, 2017
Trott
Trott previously approved these changes Jul 24, 2017
Copy link
Member

@Trott Trott left a 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?

@Trott Trott dismissed their stale review July 24, 2017 18:28

Ooof, yeah, deprecation cycle as mentioned by @jasnell...Totally +1 to moving in this direction though, let's do it!

@addaleax
Copy link
Member Author

@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.

@jasnell
Copy link
Member

jasnell commented Jul 24, 2017

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.

@addaleax
Copy link
Member Author

@jasnell done, PTAL

@@ -634,6 +634,17 @@ Type: Runtime

*Note*: change was made while `async_hooks` was an experimental API.

<a id="DEP00XX"></a>
Copy link
Contributor

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.

Copy link
Member Author

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 :)

Copy link
Member

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, thanks for clarifying :)

Copy link
Member

@jasnell jasnell left a 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

Copy link
Member

@Trott Trott left a 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

@addaleax
Copy link
Member Author

Landed in 75a19fb

@addaleax addaleax closed this Jul 26, 2017
@addaleax addaleax deleted the net-workers branch July 26, 2017 18:09
addaleax added a commit that referenced this pull request Jul 26, 2017
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>
addaleax added a commit to addaleax/node that referenced this pull request Aug 1, 2017
@addaleax addaleax mentioned this pull request Aug 1, 2017
2 tasks
addaleax added a commit that referenced this pull request Aug 1, 2017
Missed while landing 75a19fb.

Ref: #14449
PR-URL: #14576
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax added a commit to addaleax/node that referenced this pull request Nov 19, 2017
Remove the getters introduced in 75a19fb.

Refs: nodejs#14449
jasnell pushed a commit that referenced this pull request Nov 22, 2017
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. net Issues and PRs related to the net subsystem. 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.

5 participants