cluster: update deprecated API by jasnell · Pull Request #13704 · nodejs/node · GitHub

This goes along with #13702 and is part of the alternative for #13684

Mark the deprecated API non-enumerable, remove from docs, and update the deprecation message.
Technically, marking an API non-enumerable would be semver-major, but I'm proposing this as a semver-patch. The deprecated API is still there, existing code should not break as it's extremely unlikely that existing uses of the API are depending on it being enumerable.

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

Affected core subsystem(s)

cluster

If this gets push back, #13703 could be considered, as it's even less major.

LGTM

LGTM, although the deprecation message may confuse people at first glance just because it doesn't say what they should be using exitedAfterDisconnect in place of (I get that is the point though).

LGTM if Ci is green

Should we consider removing the phrase from the error code?

Unfortunately I think it's necessary for context, otherwise those seeing the deprecation error and not having any context won't really have a clue what it is about, or any ability to find out. The whole reason deprecations.md exists is to provide that context when necessary.

it can be a reference though right? like give you a url to check the extended documentation?

This is the extended documentation. This message is in deprecations.md, this is not the warning message that is printed to the console.

@jasnell I can't think of any noticeable negative consequnces of this spefic PR, but I am not fully convinced that the property should be entirely removed in 9.0 yet.

I am not sure yet that the correct action would be to remove the property entirely in 9.0.
Apart from this comment, this change looks fine.

Nit: an unfortunate decision was made to add a boolean property with the name -> there was a boolean property named

unfortunate appears a couple sentences later so we can remove the repetition of the word here without losing content

I can update that in whichever one of the PRs is landed first then cherry-pick the updated commit here.

This will cause people to see warnings like (node:4680) DeprecationWarning: Please use worker.exitedAfterDisconnect instead., which is completely uninformative.

Understood, and in general I agree that it's not ideal, but this is about finding a compromise that avoids the original problem. The reason deprecation codes were added is to provide a mechanism for finding the context if necessary.

If someone sees the warning and decides to find the context, they'll see the word anyway. The only case where hiding it here would help is when the warning is perpetually ignored.

It will be (node:4680) [DEP0007] DeprecationWarning: Please use worker.exitedAfterDisconnect instead. and that easily googleable.
We can't eliminate all references, but putting it one step further is good IMHO.
(at least it won't be repeated over and over in the logs)

It's hard for me to imagine a scenario where it would be repeated over and over in the logs, as the warning is one-time per process.

That's the paradox of this whole situation... see (content warning) Unitech/pm2#2780

Updated the deprecations.md text to use the wording suggested by @Trott here

ping @nodejs/ctc
Given the sign off, I'd like to get this landed by tomorrow, but want to make sure there are no objections

Make deprecated property non-enumerable, remove from docs

Updated the language in the docs a bit.

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

CI failure is unrelated.

PR-URL: #13704 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Make deprecated property non-enumerable, remove from docs PR-URL: #13704 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Myles Borins <myles.borins@gmail.com>

Landed in 3d2b59f, 7007f70, and f64ea03

jasnell added the cluster Issues and PRs related to the cluster subsystem. label Jun 15, 2017

nodejs-github-bot added the v8.x label Jun 15, 2017

jasnell mentioned this pull request Jun 15, 2017

cluster: remove deprecated API #13702

Closed

4 tasks

This was referenced Jun 15, 2017

cluster: tweak deprecation #13703

Closed

cluster: remove deprecated problematic API #13684

Closed

refack approved these changes Jun 15, 2017

View reviewed changes

mcollina approved these changes Jun 15, 2017

View reviewed changes

cjihrig approved these changes Jun 15, 2017

View reviewed changes

addaleax approved these changes Jun 15, 2017

View reviewed changes

MylesBorins approved these changes Jun 15, 2017

View reviewed changes

ChALkeR reviewed Jun 15, 2017

View reviewed changes

Trott reviewed Jun 15, 2017

View reviewed changes

seishun reviewed Jun 15, 2017

View reviewed changes

jasnell force-pushed the update-deprecated-api branch 2 times, most recently from f05dbdf to 8a0f530 Compare June 15, 2017 21:29

refack approved these changes Jun 17, 2017

View reviewed changes

jasnell force-pushed the update-deprecated-api branch from 8a0f530 to 704428e Compare June 20, 2017 16:03

jasnell added 3 commits June 20, 2017 09:07

doc: EOL deprecated API and update notes

0e763d9

test: use worker.exitedAfterDisconnect consistently

193b0da

cluster: make deprecated property non-enumerable

6f73f4f

Make deprecated property non-enumerable, remove from docs

jasnell force-pushed the update-deprecated-api branch from 704428e to 6f73f4f Compare June 20, 2017 16:09

refack approved these changes Jun 20, 2017

View reviewed changes

jasnell closed this Jun 20, 2017

cluster: update deprecated API by jasnell · Pull Request #13704 · nodejs/node · GitHub

Uh oh!

Uh oh!

cluster: update deprecated API - #13704

cluster: update deprecated API#13704jasnell wants to merge 3 commits intonodejs:v8.x-stagingnodejs/node:v8.x-stagingfrom jasnell:update-deprecated-apijasnell/node:update-deprecated-apiCopy head branch name to clipboard

Conversation

jasnell commented Jun 15, 2017

Checklist

Affected core subsystem(s)

Uh oh!

refack commented Jun 15, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChALkeR commented Jun 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seishun Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack Jun 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Jun 15, 2017

Uh oh!

jasnell commented Jun 19, 2017

Uh oh!

jasnell commented Jun 20, 2017

Uh oh!

jasnell commented Jun 20, 2017

Uh oh!

jasnell commented Jun 20, 2017

Uh oh!

jasnell commented Jun 20, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

cluster: update deprecated API#13704
jasnell wants to merge 3 commits into
nodejs:v8.x-stagingfrom
jasnell:update-deprecated-api

seishun Jun 15, 2017 •
edited
Loading

refack Jun 17, 2017 •
edited
Loading