Uh oh!
There was an error while loading. Please reload this page.
cluster: update deprecated API - #13704
Conversation
refack
commented
Jun 15, 2017
If this gets push back, #13703 could be considered, as it's even less major. |
cjihrig
left a comment
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Should we consider removing the phrase from the error code?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
it can be a reference though right? like give you a url to check the extended documentation?
There was a problem hiding this comment.
This is the extended documentation. This message is in deprecations.md, this is not the warning message that is printed to the console.
ChALkeR
commented
Jun 15, 2017
@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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I can update that in whichever one of the PRs is landed first then cherry-pick the updated commit here.
There was a problem hiding this comment.
This will cause people to see warnings like (node:4680) DeprecationWarning: Please use worker.exitedAfterDisconnect instead., which is completely uninformative.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That's the paradox of this whole situation... see (content warning) Unitech/pm2#2780
f05dbdf to
8a0f530Comparejasnell
commented
Jun 15, 2017
jasnell
commented
Jun 19, 2017
ping @nodejs/ctc |
8a0f530 to
704428eCompareMake deprecated property non-enumerable, remove from docs
704428e to
6f73f4fComparejasnell
commented
Jun 20, 2017
Updated the language in the docs a bit. |
jasnell
commented
Jun 20, 2017
jasnell
commented
Jun 20, 2017
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>
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>
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), orvcbuild test(Windows) passesAffected core subsystem(s)
cluster