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

doc: more stability guarantees in deprecation policy #18682

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Feb 9, 2018

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

Since this has come up a couple of times in previous, this codifies
some expectations of stability that ecosystem developers have.
@addaleax addaleax added the meta Issues and PRs related to the general management of the project. label Feb 9, 2018
@addaleax addaleax requested a review from a team February 9, 2018 16:52
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Feb 9, 2018
@addaleax addaleax mentioned this pull request Feb 9, 2018
4 tasks
Copy link
Member

@BridgeAR BridgeAR left a 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.

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.
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Contributor

@ofrobots ofrobots Feb 9, 2018

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.

Copy link
Member

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.

Copy link
Member

@jasnell jasnell Feb 9, 2018

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.

Copy link
Member

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.

Copy link
Member

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.

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

Copy link
Member

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.

Copy link
Member

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.
Copy link
Member

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.

Copy link
Member

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.

@addaleax
Copy link
Member Author

addaleax commented Feb 9, 2018

@BridgeAR Could you explain why those are your opinions?

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
Copy link
Contributor

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
Copy link
Member

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

Copy link
Member Author

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)

Copy link
Member

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.

Copy link
Member

Choose a reason for hiding this comment

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

@mcollina See #18417.

I don't think there needs to be yet another separate deprecation level, in fact I think that most of the new doc-deprecations should come with --pending-deprecation support (where possible).

Copy link
Member

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.

Copy link
Member

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.

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
Copy link
Member

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.

Copy link
Member Author

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?

* 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
Copy link
Contributor

Choose a reason for hiding this comment

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

is deemed?

@@ -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
Copy link
Member

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.
Copy link
Member

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.
Copy link
Member

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

@BridgeAR
Copy link
Member

BridgeAR commented Mar 2, 2018

Ping @addaleax

@ryzokuken
Copy link
Contributor

@addaleax what's the status on this? Are you still working on it or is it abandoned?

@addaleax addaleax added the stalled Issues and PRs that are stalled. label Jun 10, 2018
@addaleax
Copy link
Member Author

I’m closing this as stalled. Sorry I couldn’t bring this to consensus.

@addaleax addaleax closed this Jun 10, 2018
@addaleax addaleax deleted the deprecation-policy branch June 10, 2018 19:56
@ryzokuken
Copy link
Contributor

No worries. Thanks for taking a stab at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project. stalled Issues and PRs that are stalled.
Projects
None yet
Development

Successfully merging this pull request may close these issues.