-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc: more stability guarantees in deprecation policy #18682
Conversation
Since this has come up a couple of times in previous, this codifies some expectations of stability that ecosystem developers have.
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.
The main part is LGTM but I disagree with the last two paragraphs.
COLLABORATOR_GUIDE.md
Outdated
In particular, a Runtime deprecation should *not* be added to an API that | ||
that requires only trivial maintenance, or only because it has | ||
never been documented or has only accidentally been exposed and there is | ||
usage of said API in existing ecosystem code. |
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 disagree that a runtime deprecation should not be used in case an API has never been documented before. Especially in such a case I would want a runtime deprecation as otherwise it is highly unlikely that anyone will ever realize that a specific code is actually deprecated and was never documented.
Also accidentally exposing a API to the ecosystem should receive a runtime deprecation out of my perspective.
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.
otherwise it is highly unlikely that anyone will ever realize that a specific code is actually deprecated and was never documented.
I fail to see how that is a bad thing?
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 fail to see how that is a bad thing?
I am closer to the viewpoint that @addaleax has on this, i.e. this is not necessarily a bad thing. However, one has to recognize that, over time, we end up with a large number of documentation-only deprecated stuff (i.e. bloat). That's the downside, and I personally think it is okay. I don't think we are too bloated ATM.
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.
If we add this, I would like a note that runtime deprecation may occur if a previously undocumented or accidentally exposed API is given a documented and intentional API.
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 would agree with @bmeck's caveat here. If there is a supported replacement, we ought to be able to runtime deprecate the trivial unsupported thing.
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.
and there is usage
There is noticeable/significant usage?
I don't think a single abadoned line of code on GitHub would justify keeping the API.
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 think one reason for the different point of views here are what we understand with a breaking change and more importantly: how "bad" a runtime deprecation is for each of us. For me a runtime deprecation means there is just something logged so people get notified that it is not recommended to use a specific API anymore / in a specific way. It does not even mean that something might or might not be removed. I feel like that is nothing bad - almost ever.
Without that notification we reach probably less than a thousandths of the users and probably especially those for whom the deprecation is actually meant for. A documentation deprecation is only useful out of my perspective when we want to only prevent new people from using something. But even that is often probably not a reason for people to not use something, especially as a lot of people do not read a documentation properly and they might just check a online example that they found somewhere else than in the official documentation.
Instead we are able to shift code to become better and to outline that things are not ideal and I think that is something great.
So I feel like we should always have the possibility of runtime deprecating things that were never meant to be exposed or that were never documented. In some cases it might be useful to add a official API instead, on other cases there might be nothing to do besides deprecating it to tell people to better switch and in even more cases there are probably other solutions.
Documenting something and than runtime deprecating is the opposite of what I would see as logical because a lot more people will actually read the documentation about the deprecation (especially new people) than people who try to use a API they stumble upon somewhere in the Node.js source code. So for me it is like the opposite step of what is meant to be achieved.
Deciding on what to do should out of my perspective be individual per case. I do not want to have a strict limitation one way or the other.
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 would prefer those non-documented APIs to be deprecated, and add the relevant public APIs to keep solving the same use case.
I think nobody's disagreeing with that. :)
If a non-documented API is not very widespread in the ecosystem, it's the right moment to runtime deprecate it (and potentially replace with a new one), because it has a chance to become popular.
Okay, I can see that. Let me think about it.
For me a runtime deprecation means there is just something logged so people get notified that it is not recommended to use a specific API anymore / in a specific way.
The feedback that we've gotten from the Buffer deprecation discussions is different: A runtime deprecation is functional breakage in itself.
A documentation deprecation is only useful out of my perspective when we want to only prevent new people from using something.
I agree.
Documenting something and than runtime deprecating is the opposite of what I would see as logical because a lot more people will actually read the documentation about the deprecation (especially new people) than people who try to use a API they stumble upon somewhere in the Node.js source code.
Please note that that's not what this PR is suggesting; I am totally fine with considering an undocumented API to be implicitly docs-deprecated for the purposes of this policy.
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.
Please note that that's not what this PR is suggesting; I am totally fine with considering an undocumented API to be implicitly docs-deprecated for the purposes of this policy.
I just mentioned that because it was brought up.
The feedback that we've gotten from the Buffer deprecation discussions is different: A runtime deprecation is functional breakage in itself.
Well, I personally think that is a strange view. The code itself still works perfectly fine, so having a warning is something that I would actually want. And especially the Buffer deprecation is something where there are a lot of different opinions.
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.
This paragraph contains one sentence that seems overly long and perhaps difficult to understand. Consider breaking it into multiple sentences. Perhaps something like this?:
A Runtime deprecation should not be added to an API that
requires only trivial maintenance. A Runtime deprecation should
not be added simply because an API has never been documented
(or has only accidentally been exposed) if there is significant usage
of the API in the ecosystem.
|
||
In addition, a Documentation-Only deprecation may be issued if it is desirable | ||
to recommend a single canonical way out of multiple different ones | ||
which achieve a particular goal with Node.js. |
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.
This is probably correct in most cases but I would not want to strictly limit it to documentation only. I would rather keep that part open and individually decide what to do.
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.
Can we please change that to:
Possible reasons for a Documentation-Only deprecation can be:
* It is desirable to recommend a single canonical way out of multiple different ones which achieve a particular goal with Node.js.
* An existing API provides very little value to users and it should not be implemented anymore but keeping it does not hurt either.
@BridgeAR Could you explain why those are your opinions? |
COLLABORATOR_GUIDE.md
Outdated
which achieve a particular goal with Node.js. | ||
|
||
In particular, a Runtime deprecation should *not* be added to an API that | ||
that requires only trivial maintenance, or only because it has |
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.
A nit: that that
-> that
.
Documentation-only deprecations may trigger a runtime warning when launched | ||
with [`--pending-deprecation`][] flag (or its alternative, | ||
with [`--pending-deprecation`][] flag (or its alternative, the |
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.
This paragraph says that "Documentation-Only" is a deprecation level, however the "pending" implies that it has not been deprecated yet.
I do not think this terminology is consistent with the --pending-deprecation
flag.
IMHO --pending-deprecation
needs to be better explained as another level, something like "pending a runtime deprecation".
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.
@mcollina I don't think we can rename the flag anymore, although I agree that the name is unfortunate. I think the text is explicit enough about what the flag does, but I'm not sure we should codify that --pending-deprecations
means that we are definitely going to runtime-deprecate something at some point?
(For context, this text is from the rather recent #18433, /cc @ChALkeR)
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 think we should name "pending runtime deprecation" as its own deprecation level. If not, we should try to define this better. Before this change, "doc-only deprecations" were a step towards "runtime deprecations" (in most cases). after this change, they became their own thing, which not necessarily would lead to a more severe deprecation. I think there is a space for something in between, were a doc-only deprecation would lead to a runtime deprecation.
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.
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'm not sure we should codify that --pending-deprecations means that we are definitely going to runtime-deprecate something at some point?
That's what it means though, doesn't it? We currently use it for unsafe new Buffer()
calls and I believe the plan is to warn more forcefully in the future.
Documentation-only deprecations are more for steering people towards the canonical solution. --pending-deprecations
doesn't need to warn about that because we're not going to runtime-deprecate it.
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.
documentation-only could but doesn't mean must be runtime deprecated. The only time it definitely should mean runtime deprecation is if we ultimately plan to end-of-life the code.
pending-deprecation should mean we'll eventually runtime deprecate.
Any documentation-only deprecation that we expect to move to runtime deprecation in the future should emit pending deprecation.
COLLABORATOR_GUIDE.md
Outdated
undetected bugs or security issues in existing code. | ||
|
||
* Breaking the API makes way for simplification of its implementation that is | ||
significant enough to make maintainability of Node.js easier, or enables |
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.
significant enough to make maintainability of Node.js easier
Compared to the ecosystem usage / taking ecosystem into account / something like that?
E.g. something huge and ugly but widely used is still kept, but something that adds only a bit of churn but isn't used by anyone can be runtime-deprecated quickly.
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.
@ChALkeR I think this is at least implicitly covered by the paragraph below …
Either way, this doesn’t feel like the right place for that – this is listing the reasons to deprecate, and all decisions to deprecate should take ecosystem usage into account, right?
COLLABORATOR_GUIDE.md
Outdated
* Node.js will no longer be able to support the API, for example if required | ||
code is being removed in an upstream project. | ||
|
||
* The API is too hard or impossible to use correctly, increasing the risk of |
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.
is deemed
?
COLLABORATOR_GUIDE.md
Outdated
@@ -391,6 +391,27 @@ Runtime Deprecations and End-of-life APIs (internal or public) must be | |||
handled as semver-major changes unless there is TSC consensus to land the | |||
deprecation as a semver-minor. | |||
|
|||
Possible reasons for Runtime deprecations can be: | |||
|
|||
* Node.js will no longer be able to support the API, for example if required |
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.
Nit: comma after example
* Node.js will no longer be able to support the API, for example, if required
|
||
In addition, a Documentation-Only deprecation may be issued if it is desirable | ||
to recommend a single canonical way out of multiple different ones | ||
which achieve a particular goal with Node.js. |
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.
Can we please change that to:
Possible reasons for a Documentation-Only deprecation can be:
* It is desirable to recommend a single canonical way out of multiple different ones which achieve a particular goal with Node.js.
* An existing API provides very little value to users and it should not be implemented anymore but keeping it does not hurt either.
requires only trivial maintenance. A Runtime deprecation should | ||
not be added simply because an API has never been documented | ||
(or has only accidentally been exposed) if there is significant usage | ||
of the API in the ecosystem. |
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.
Can this please be changed to e.g.:
In case there is significant usage of an API in the ecosystem a Runtime deprecation should
not be added simply because the API has never been documented (or has only accidentally
been exposed) without existing alternative. The deprecation notice has to reference the
alternative in such a case.
And I think it would be best to move this up below the last entry of Possible reasons for Runtime deprecations can be:
.
Ping @addaleax |
@addaleax what's the status on this? Are you still working on it or is it abandoned? |
I’m closing this as stalled. Sorry I couldn’t bring this to consensus. |
No worries. Thanks for taking a stab at it. |
Since this has come up a couple of times in previous, this codifies
some expectations of stability that ecosystem developers have.
Checklist
Affected core subsystem(s)
doc