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.

Sign up for GitHub

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

Jump to bottom

doc: more stability guarantees in deprecation policy #18682

Closed
wants to merge 2 commits into from
Closed

doc: more stability guarantees in deprecation policy #18682

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
109af15
doc: more stability guarantees in deprecation policy
addaleax Feb 9, 2018
d5d524e
[squash] nits/suggestions
addaleax Feb 13, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 28 additions & 6 deletions COLLABORATOR_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,13 +364,13 @@ there is expected impact on the user community.

Node.js uses three Deprecation levels:

* *Documentation-Only Deprecation* refers to elements of the Public API that are
being staged for deprecation in a future Node.js major release. An explicit
notice indicating the deprecated status is added to the API documentation
but no functional changes are implemented in the code. There will be no
runtime deprecation warnings emitted for such deprecations by default.
* *Documentation-Only Deprecation* refers to elements of the Public API that
should be avoided by developers. An explicit notice indicating the deprecation
status is added to the API documentation but no functional changes are
implemented in the code.
There will be no warnings emitted for such deprecations at runtime by default.
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.

`NODE_PENDING_DEPRECATION=1` environment variable).

* *Runtime Deprecation* refers to the use of process warnings emitted at
Expand All @@ -391,6 +391,28 @@ 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
code is being removed in an upstream project.

* The API is deemed too hard or impossible to use correctly, increasing the
risk of 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
implementation of features that would otherwise be impossible.

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.


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


All Documentation-Only and Runtime deprecations will be assigned a unique
identifier that can be used to persistently refer to the deprecation in
documentation, emitted process warnings, or errors thrown. Documentation for
Expand Down