meta: deprecation policy #7964

Closed
wants to merge 1 commit into
from

Conversation

@jasnell
Member

jasnell commented Aug 3, 2016

Checklist
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

meta

Description of change

First step at documenting the deprecation requirements.
Refs: #7955
/cc @nodejs/ctc

@jasnell jasnell added the meta label Aug 3, 2016

@Trott

View changes

COLLABORATOR_GUIDE.md
@@ -73,6 +73,70 @@ All pull requests that modify executable code should be subjected to
continuous integration tests on the
[project CI server](https://ci.nodejs.org/).
+## Semver-major Changes

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: Use "Breaking Changes" or some other generally-used terminology? My experience, at least, is that while "semantic versioning" is widely understood, the term "semver major" is confusing to people who have not heard it before. I have not encountered it outside of nodejs repos.

@Trott

Trott Aug 3, 2016

Member

Nit: Use "Breaking Changes" or some other generally-used terminology? My experience, at least, is that while "semantic versioning" is widely understood, the term "semver major" is confusing to people who have not heard it before. I have not encountered it outside of nodejs repos.

@Trott

View changes

COLLABORATOR_GUIDE.md
@@ -73,6 +73,70 @@ All pull requests that modify executable code should be subjected to
continuous integration tests on the
[project CI server](https://ci.nodejs.org/).
+## Semver-major Changes
+
+Changes to API arguments, return values, error handling, timing, and

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

I would stick with a general definition of a breaking change. I don't think the description here is accurate. For example, adding an API argument could be semver-minor if it does not change behavior in code that does not use the new argument. (EDIT: Yeah, I know you wrote "changing" and it's questionable whether "adding" is a subset of "changing" or is separate from "changing". But to me, that potential confusion is an argument supporting the approach I suggest.)

If you do want to include a list of examples of the types of things that are breaking changes, I would put that after a general definition rather than coming out of the gate with it.

@Trott

Trott Aug 3, 2016

Member

I would stick with a general definition of a breaking change. I don't think the description here is accurate. For example, adding an API argument could be semver-minor if it does not change behavior in code that does not use the new argument. (EDIT: Yeah, I know you wrote "changing" and it's questionable whether "adding" is a subset of "changing" or is separate from "changing". But to me, that potential confusion is an argument supporting the approach I suggest.)

If you do want to include a list of examples of the types of things that are breaking changes, I would put that after a general definition rather than coming out of the gate with it.

This comment has been minimized.

@jasnell

jasnell Aug 3, 2016

Member

ok, yeah, I struggled a bit with this part as the rules are not yet completely clear.

@jasnell

jasnell Aug 3, 2016

Member

ok, yeah, I struggled a bit with this part as the rules are not yet completely clear.

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Maybe something like this?:

Backwards-incompatible changes may land on the master branch at any time. Prior to landing on master, the changes must undergo the usual review process that applies to all changes. Additionally, they must be approved by at least two members of the Core Technical Committee.

Optional additional material:

Examples of backwards-incompatible changes include: (insert bulleted list here)

@Trott

Trott Aug 3, 2016

Member

Maybe something like this?:

Backwards-incompatible changes may land on the master branch at any time. Prior to landing on master, the changes must undergo the usual review process that applies to all changes. Additionally, they must be approved by at least two members of the Core Technical Committee.

Optional additional material:

Examples of backwards-incompatible changes include: (insert bulleted list here)

@Trott

View changes

COLLABORATOR_GUIDE.md
+
+Adding or removing errors thrown or reported by an API, or altering the timing
+and non-internal side effects of the API can be done *without* a Deprecation
+cycle but *must* be landed as semver-major changes. For now, this also includes

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: Remove "For now". (Everything in the entire document is "for now" and subject to change.)

@Trott

Trott Aug 3, 2016

Member

Nit: Remove "For now". (Everything in the entire document is "for now" and subject to change.)

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: Indicate that "Deprecation cycle" is defined below and/or include an internal link to the definition.

@Trott

Trott Aug 3, 2016

Member

Nit: Indicate that "Deprecation cycle" is defined below and/or include an internal link to the definition.

@cjihrig

View changes

COLLABORATOR_GUIDE.md
+non-internal side effects must be handled as semver-major changes. These may be
+landed in master at any time with sufficient CTC review and approval.
+
+If it becomes necessary to change an APIs arguments or return values (the

This comment has been minimized.

@cjihrig

cjihrig Aug 3, 2016

Contributor

APIs -> API's

@cjihrig

cjihrig Aug 3, 2016

Contributor

APIs -> API's

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Alternatively? change the arguments or return values of an API

@Trott

Trott Aug 3, 2016

Member

Alternatively? change the arguments or return values of an API

@Trott

View changes

COLLABORATOR_GUIDE.md
+Changes that add new events to EventEmitter implementations may be landed as
+semver-minor changes. *Removing* events, or altering the semantics of existing
+events *must* be landed as semver-major and *must* go through a Deprecation
+cycle.

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: Indicate that "Deprecation cycle" is defined below and/or include an internal link to the definition.

@Trott

Trott Aug 3, 2016

Member

Nit: Indicate that "Deprecation cycle" is defined below and/or include an internal link to the definition.

@cjihrig

View changes

COLLABORATOR_GUIDE.md
+API "signature"), the existing API *must* be Deprecated *first* and the new
+API either introduced in parallel or added after the next major Node.js version
+following the Deprecation as a replacement for the Deprecated API. In other
+words, existing API signatures *must not* change without a Deprecation.

This comment has been minimized.

@cjihrig

cjihrig Aug 3, 2016

Contributor

Maybe say "change in a backwards incompatible fashion"

@cjihrig

cjihrig Aug 3, 2016

Contributor

Maybe say "change in a backwards incompatible fashion"

@Trott

View changes

COLLABORATOR_GUIDE.md
+## Deprecations
+
+Deprecation refers to the identification of APIs that should no longer be used
+and that will no longer be supported in Node.js.

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: I'd either remove the second part about not being supported or else describe what not being supported means. sys is deprecated but if using it breaks, we're going to fix it. Is that not support?

@Trott

Trott Aug 3, 2016

Member

Nit: I'd either remove the second part about not being supported or else describe what not being supported means. sys is deprecated but if using it breaks, we're going to fix it. Is that not support?

@Trott

View changes

COLLABORATOR_GUIDE.md
+Node.js uses three fundamental Deprecation levels:
+
+* Soft-Deprecation (or Docs-only deprecation) refers to elements of the Node.js
+ public API that are being staged for deprecation in a future Node.js major.

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: major -> major version or major release or something like that.

@Trott

Trott Aug 3, 2016

Member

Nit: major -> major version or major release or something like that.

@Trott

View changes

COLLABORATOR_GUIDE.md
+ public API that are being staged for deprecation in a future Node.js major.
+ 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 warning).

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: Make the parenthetical it's own sentence without the parentheses. It deserves emphasis.

@Trott

Trott Aug 3, 2016

Member

Nit: Make the parenthetical it's own sentence without the parentheses. It deserves emphasis.

@Trott

View changes

COLLABORATOR_GUIDE.md
+ documentation *but no functional changes are implemented in the code* (there
+ will be no runtime deprecation warning).
+
+* Hard-Deprecation refers to the use of warnings or errors being reported at

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: remove or errors. It's covered by the CLI flag thing a sentence or two later. Having it appear before that is confusing. "Oh no! Will it print a warning or throw an error? It might do either ! AHHHHH!!!"

@Trott

Trott Aug 3, 2016

Member

Nit: remove or errors. It's covered by the CLI flag thing a sentence or two later. Having it appear before that is confusing. "Oh no! Will it print a warning or throw an error? It might do either ! AHHHHH!!!"

@Trott

View changes

COLLABORATOR_GUIDE.md
+* End-of-life refers to APIs that have gone through Hard-Deprecation and are
+ ready to be removed from Node.js entirely.
+
+All Deprecations *must* be handled as semver-major changes and are subject to

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: remove "and are subject to review and approval by the CTC". That's a true statement, but:

  • It's covered by the fact that it's a semver-major change. (Repeat nit: "breaking change" instead?)
  • The next sentence says "This means" but the "this" is referring to the semver-major status, not the CTC review stuff, and so having the CTC stuff there adds confusion as one reads the paragraph.
@Trott

Trott Aug 3, 2016

Member

Nit: remove "and are subject to review and approval by the CTC". That's a true statement, but:

  • It's covered by the fact that it's a semver-major change. (Repeat nit: "breaking change" instead?)
  • The next sentence says "This means" but the "this" is referring to the semver-major status, not the CTC review stuff, and so having the CTC stuff there adds confusion as one reads the paragraph.
@Trott

View changes

COLLABORATOR_GUIDE.md
+can be landed in master at any time with appropriate review and approval, such
+changes are only permitted to be delivered in new major releases.
+
+A "Deprecation cycle" is one full Node.js major release.

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: "..one full Node.js major release during which a feature has been in one of the three Deprecation levels." This is something that logically follows from the text (advancement to any Deprecation level is semver-major, so it can only happen at the start of a major release, and it cannot change again until the next major releease) but it is good to spell out.

@Trott

Trott Aug 3, 2016

Member

Nit: "..one full Node.js major release during which a feature has been in one of the three Deprecation levels." This is something that logically follows from the text (advancement to any Deprecation level is semver-major, so it can only happen at the start of a major release, and it cannot change again until the next major releease) but it is good to spell out.

@Trott

View changes

COLLABORATOR_GUIDE.md
+No APIs can be moved to End-of-life without first having gone through a
+Hard-Deprecation cycle.
+
+A best effort should be taken to communicate pending Deprecations and associated

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: should be taken -> will be made

@Trott

Trott Aug 3, 2016

Member

Nit: should be taken -> will be made

@Trott

View changes

COLLABORATOR_GUIDE.md
+mitigations with the ecosystem as soon as possible (preferably when the pull
+request adding the Deprecation is landed in master).
+
+Proposals to request Deprecation of an API should include an explanation of the

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: Remove this sentence. (Do we really have a problem of a deluge of deprecation requests that don't explain the reasons?)

@Trott

Trott Aug 3, 2016

Member

Nit: Remove this sentence. (Do we really have a problem of a deluge of deprecation requests that don't explain the reasons?)

@Trott

View changes

COLLABORATOR_GUIDE.md
+
+Proposals to request Deprecation of an API should include an explanation of the
+reasons, benefits, and mitigations available to potential users.
+Soft-Deprecation should be favored over Hard-Deprecation unless it is clear

This comment has been minimized.

@Trott

Trott Aug 3, 2016

Member

Nit: remove unless it is clear that the potential impact on the ecosystem is minimal. If you want to leave it squishy, add should generally although I think should is plenty squishy enough. We have a so-so track record (or at least a so-so reputation, deserved or not) on predicting ecosystem impact, and we should encourage caution in my opinion.

@Trott

Trott Aug 3, 2016

Member

Nit: remove unless it is clear that the potential impact on the ecosystem is minimal. If you want to leave it squishy, add should generally although I think should is plenty squishy enough. We have a so-so track record (or at least a so-so reputation, deserved or not) on predicting ecosystem impact, and we should encourage caution in my opinion.

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 3, 2016

Member

Tons of nits, but no major objections. LGTM.

Member

Trott commented Aug 3, 2016

Tons of nits, but no major objections. LGTM.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 3, 2016

Member

Updated!

Member

jasnell commented Aug 3, 2016

Updated!

@addaleax

View changes

COLLABORATOR_GUIDE.md
+* Soft-Deprecation (or Docs-only deprecation) refers to elements of the Node.js
+ 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.

This comment has been minimized.

@addaleax

addaleax Aug 3, 2016

Member

missing closing *?

@addaleax

addaleax Aug 3, 2016

Member

missing closing *?

This comment has been minimized.

@jasnell

jasnell Aug 3, 2016

Member

Fixed!

@jasnell

jasnell Aug 3, 2016

Member

Fixed!

@ChALkeR

View changes

COLLABORATOR_GUIDE.md
+
+Exception to this rule is given in the following cases:
+
+* Adding or removing errors thrown or reported by an API;

This comment has been minimized.

@ChALkeR

ChALkeR Aug 3, 2016

Member

What exactly does this mean?
Would changes on the V8 side that forced Buffer changes upon be treated as such exceptions?
Would entirely removing a module count as «adding an error thrown»? 😉
What about #6123, for example — does that count as «adding an error thrown»?
What about dropping an OS support?

@ChALkeR

ChALkeR Aug 3, 2016

Member

What exactly does this mean?
Would changes on the V8 side that forced Buffer changes upon be treated as such exceptions?
Would entirely removing a module count as «adding an error thrown»? 😉
What about #6123, for example — does that count as «adding an error thrown»?
What about dropping an OS support?

This comment has been minimized.

@jasnell

jasnell Aug 3, 2016

Member

Perhaps a statement such as, "Additional exceptions may be made following CTC review and approval."??

@jasnell

jasnell Aug 3, 2016

Member

Perhaps a statement such as, "Additional exceptions may be made following CTC review and approval."??

@Fishrock123

View changes

COLLABORATOR_GUIDE.md
+
+Examples of breaking changes include, but are not necessarily limited to,
+removal or redefinition of existing API arguments, changing return values,
+adding or removing errors, altering the timing, and changing the non-internal

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 4, 2016

Member

I'm not sure that removing errors is a breaking change.

Removing an error because something now works seems like a minor feature addition

@Fishrock123

Fishrock123 Aug 4, 2016

Member

I'm not sure that removing errors is a breaking change.

Removing an error because something now works seems like a minor feature addition

This comment has been minimized.

@jasnell

jasnell Aug 4, 2016

Member

In general it should be, I think, but I've come across cases (as awful as it is) where thrown errors are used for flow control. Removing an expected error, then, can lead to code no longer functioning correctly. For instance, glob currently appears to rely on ENAMETOOLONG errors to know when to bail out to avoid recursing endlessly in certain situations. Given how sensitive error handling is, it's often best to err on the side of caution than to make assumptions about what things ought to be.

@jasnell

jasnell Aug 4, 2016

Member

In general it should be, I think, but I've come across cases (as awful as it is) where thrown errors are used for flow control. Removing an expected error, then, can lead to code no longer functioning correctly. For instance, glob currently appears to rely on ENAMETOOLONG errors to know when to bail out to avoid recursing endlessly in certain situations. Given how sensitive error handling is, it's often best to err on the side of caution than to make assumptions about what things ought to be.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 8, 2016

Member

hmm, ok

@Fishrock123

Fishrock123 Aug 8, 2016

Member

hmm, ok

This comment has been minimized.

@jasnell

jasnell Aug 8, 2016

Member

(I'm not particularly happy about this part, fwiw...)

@jasnell

jasnell Aug 8, 2016

Member

(I'm not particularly happy about this part, fwiw...)

This comment has been minimized.

@trevnorris

trevnorris Jan 4, 2017

Contributor

from past discussion doesn't this also include changing error messages? I see your exception below

@trevnorris

trevnorris Jan 4, 2017

Contributor

from past discussion doesn't this also include changing error messages? I see your exception below

This comment has been minimized.

@jasnell

jasnell Jan 4, 2017

Member

+1 ... again, consider this entire PR to be a strawman.

@jasnell

jasnell Jan 4, 2017

Member

+1 ... again, consider this entire PR to be a strawman.

@Fishrock123

View changes

COLLABORATOR_GUIDE.md
+All Deprecations *must* be handled as breaking (semver-major) changes. This
+means that while a PR adding a Deprecation can be landed in master at any time
+with appropriate review and approval, such changes are only permitted to be
+delivered in new major releases.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 4, 2016

Member

I think this should have the exception of what I discussed in the CTC meeting, although it may not need to be written given it should not be something that happens under regular circumstances.

@Fishrock123

Fishrock123 Aug 4, 2016

Member

I think this should have the exception of what I discussed in the CTC meeting, although it may not need to be written given it should not be something that happens under regular circumstances.

This comment has been minimized.

@jasnell

jasnell Aug 4, 2016

Member

Can you be more specific as to what that exception was? I do not remember the details.

@jasnell

jasnell Aug 4, 2016

Member

Can you be more specific as to what that exception was? I do not remember the details.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 8, 2016

Member
  • native realpath is accepted as a semver-major, we know it changes internals but do not find any problems
  • it releases in a semver-major (v6)
  • afterwards it is discovered that it has side-effects which break too much and could have been prevented
  • a revert is made but we still plan to do this in some form in the future

Intention was already made in this major, unbreaking something but then printing a deprecation for it isn't breaking at this point, it's still unbreaking. We should be able to do that imo.

@Fishrock123

Fishrock123 Aug 8, 2016

Member
  • native realpath is accepted as a semver-major, we know it changes internals but do not find any problems
  • it releases in a semver-major (v6)
  • afterwards it is discovered that it has side-effects which break too much and could have been prevented
  • a revert is made but we still plan to do this in some form in the future

Intention was already made in this major, unbreaking something but then printing a deprecation for it isn't breaking at this point, it's still unbreaking. We should be able to do that imo.

This comment has been minimized.

@jasnell

jasnell Aug 8, 2016

Member

Agreed but it's going to be difficult to succinctly capture the nuance
there... hmm.. will give the wording around some more thought.

NOTE(@trevnorris): removed large email body from responding to this comment.

@jasnell

jasnell Aug 8, 2016

Member

Agreed but it's going to be difficult to succinctly capture the nuance
there... hmm.. will give the wording around some more thought.

NOTE(@trevnorris): removed large email body from responding to this comment.

@targos

View changes

COLLABORATOR_GUIDE.md
+Exception to this rule is given in the following cases:
+
+* Adding or removing errors thrown or reported by an API;
+* Changing error messages;

This comment has been minimized.

@targos

targos Aug 4, 2016

Member

What about error messages that we cannot control ? I'm thinking about the messages from V8 that often change between versions.

@targos

targos Aug 4, 2016

Member

What about error messages that we cannot control ? I'm thinking about the messages from V8 that often change between versions.

This comment has been minimized.

@jasnell

jasnell Aug 4, 2016

Member

Good question. We can really only apply this policy to code we directly control. If v8 changes their error handling at all in a patch or minor, there's not going to be much we can do about it. I'll make a note to clarify this.

@jasnell

jasnell Aug 4, 2016

Member

Good question. We can really only apply this policy to code we directly control. If v8 changes their error handling at all in a patch or minor, there's not going to be much we can do about it. I'll make a note to clarify this.

@Trott

View changes

COLLABORATOR_GUIDE.md
+changes to an API are necessary, the existing API *must* be Deprecated *first*
+and the new API either introduced in parallel or added after the next major
+Node.js version following the Deprecation as a replacement for the Deprecated
+API. In other words, existing API's *must not* change without a Deprecation.

This comment has been minimized.

@Trott

Trott Aug 4, 2016

Member

API's -> APIs

APIs = plural of API

  • That group of APIs should drink 400ml of water every day.

API's = possessive

  • That API's haircut is amazing!
@Trott

Trott Aug 4, 2016

Member

API's -> APIs

APIs = plural of API

  • That group of APIs should drink 400ml of water every day.

API's = possessive

  • That API's haircut is amazing!

This comment has been minimized.

@jasnell

jasnell Aug 4, 2016

Member

I'm getting flashbacks of my fourth grade teacher ;-) ... that dang ' is my nemesis I swear

@jasnell

jasnell Aug 4, 2016

Member

I'm getting flashbacks of my fourth grade teacher ;-) ... that dang ' is my nemesis I swear

@Trott

View changes

COLLABORATOR_GUIDE.md
+
+## Deprecations
+
+Deprecation refers to the identification of API's that should no longer be used

This comment has been minimized.

@Trott

Trott Aug 4, 2016

Member

API's -> APIs

@Trott

Trott Aug 4, 2016

Member

API's -> APIs

@Trott

View changes

COLLABORATOR_GUIDE.md
+ cause the Node.js process to exit. As with Soft-Deprecation, the documentation
+ for the API must be updated to clearly indicate the deprecated status.
+
+* End-of-life refers to API's that have gone through Hard-Deprecation and are

This comment has been minimized.

@Trott

Trott Aug 4, 2016

Member

API's -> APIs

@Trott

Trott Aug 4, 2016

Member

API's -> APIs

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 4, 2016

Member

fixed nits and added some additional discussion detail. PTAL

Member

jasnell commented Aug 4, 2016

fixed nits and added some additional discussion detail. PTAL

@ChALkeR ChALkeR referenced this pull request Aug 5, 2016

Closed

buffer: throw on negative .allocUnsafe() argument #7079

3 of 3 tasks complete
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 5, 2016

Member

Refs: #7912

Member

jasnell commented Aug 5, 2016

Refs: #7912

@Fishrock123

View changes

COLLABORATOR_GUIDE.md
+sufficient review by collaborators and approval of at least two CTC members.
+
+Examples of breaking changes include, but are not necessarily limited to,
+removal or redefinition of existing API arguments, changing return values,

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 8, 2016

Member

except when return values do not currently exist

@Fishrock123

Fishrock123 Aug 8, 2016

Member

except when return values do not currently exist

@Fishrock123

View changes

COLLABORATOR_GUIDE.md
+without a [Deprecation cycle](#deprecation-cycle).
+
+From time-to-time, in particularly exceptional cases, the CTC may be asked to
+consider and approval additional exceptions to this rule.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 8, 2016

Member

approve*

@Fishrock123

Fishrock123 Aug 8, 2016

Member

approve*

@Fishrock123

View changes

COLLABORATOR_GUIDE.md
+changes *do*, in fact, break existing code, a decision may be made to revert
+those changes either temporarily or permanently. However, the decision to
+revert or not can often be based on many complex factors that are not easily
+codified.

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 8, 2016

Member

or, more often it may be tagged as semver-major after the fact if appropriate per the above guidlines

@Fishrock123

Fishrock123 Aug 8, 2016

Member

or, more often it may be tagged as semver-major after the fact if appropriate per the above guidlines

@jrjohnson

View changes

COLLABORATOR_GUIDE.md
+delivered in new major releases.
+
+<a id="deprecation-cycle"></a>
+A "Deprecation cycle" is one full Node.js major release during which an API

This comment has been minimized.

@jrjohnson

jrjohnson Aug 11, 2016

It seems to me that it would be very helpful if deprecation cycles were connected to LTS releases such that end of life could never be reached without a hard deprecation in an LTS release. This makes it possible to jump between LTS versions without needing to make interim stops to check for deprecations on the way.

@jrjohnson

jrjohnson Aug 11, 2016

It seems to me that it would be very helpful if deprecation cycles were connected to LTS releases such that end of life could never be reached without a hard deprecation in an LTS release. This makes it possible to jump between LTS versions without needing to make interim stops to check for deprecations on the way.

This comment has been minimized.

@Trott

Trott Aug 11, 2016

Member

If I understand correctly, EOL means it's still there, just that we aren't going to do much to support it because it is going to be removed in the next major release.

Nothing can go to EOL without going through hard deprecation first.

So, (again, if I understand correctly) the worst case scenario is:

  • LTS (let's say v14 just to have numbers to work with) release has feature X
  • non-LTS v15 hard deprecates the feature, so that means it prints a warning at runtime when the feature is used
  • LTS v16 moves the feature to EOL. It's still there but the runtime warnings from v15 when using the feature might be more strongly worded and a date has been set for its removal from Node.js (which would presumably be the v17 release).
  • non-LTS v17 is where the feature is finally removed and your code that depends on it no longer works at all.

@jasnell Am I correct above?

@jrjohnson If I am correct above, does that more-or-less match what you're proposing? Or is there still a change you'd like to see?

@Trott

Trott Aug 11, 2016

Member

If I understand correctly, EOL means it's still there, just that we aren't going to do much to support it because it is going to be removed in the next major release.

Nothing can go to EOL without going through hard deprecation first.

So, (again, if I understand correctly) the worst case scenario is:

  • LTS (let's say v14 just to have numbers to work with) release has feature X
  • non-LTS v15 hard deprecates the feature, so that means it prints a warning at runtime when the feature is used
  • LTS v16 moves the feature to EOL. It's still there but the runtime warnings from v15 when using the feature might be more strongly worded and a date has been set for its removal from Node.js (which would presumably be the v17 release).
  • non-LTS v17 is where the feature is finally removed and your code that depends on it no longer works at all.

@jasnell Am I correct above?

@jrjohnson If I am correct above, does that more-or-less match what you're proposing? Or is there still a change you'd like to see?

This comment has been minimized.

@jasnell

jasnell Aug 11, 2016

Member

@Trott .. yes, this is what I had in mind.

@jasnell

jasnell Aug 11, 2016

Member

@Trott .. yes, this is what I had in mind.

This comment has been minimized.

@jrjohnson

jrjohnson Aug 11, 2016

Ok - That makes perfect sense to me and is even MORE than I was hoping for. Thanks.

@jrjohnson

jrjohnson Aug 11, 2016

Ok - That makes perfect sense to me and is even MORE than I was hoping for. Thanks.

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@Fishrock123

Fishrock123 Aug 24, 2016

Member

I see/hear a consistent confusion about what the terms "hard" & "soft" deprecate mean without detailed context.

I think we should use "docs" and "runtime" deprecate as our primary way of communicating these, but still also note what the others mean in the docs for context.

i.e.

"Runtime ("hard") deprecation ..."

Member

Fishrock123 commented Aug 24, 2016

I see/hear a consistent confusion about what the terms "hard" & "soft" deprecate mean without detailed context.

I think we should use "docs" and "runtime" deprecate as our primary way of communicating these, but still also note what the others mean in the docs for context.

i.e.

"Runtime ("hard") deprecation ..."

@Trott

This comment has been minimized.

Show comment
Hide comment
@Trott

Trott Aug 24, 2016

Member

The comment above from @Fishrock123 addresses confusion around "soft" and "hard" deprecate, and I like his suggestion. More clarity, less jargon.

It does leave the unaddressed the third phase (what's called end-of-life) in the doc. While end-of-life has a well-understood meaning, I think we should avoid it here as we're not quite using it the way others do, at least in my experience.

I propose that we just have two phases and indicate that the second phase ("hard" deprecation or, in @Fishrock123's suggestions, "runtime" deprecation) must last for two semver major releases. Because as far as I can tell, there's no difference between "hard" deprecation and "end-of-life" other than that "end-of-life" has to come after a major release in "hard" deprecation. So just make it two major releases and all is clear to the reader. Two labels, instead of three, less confusion, more clarity.

This all assumes I haven't overlooked something with "end-of-life" or otherwise misunderstood it.

Member

Trott commented Aug 24, 2016

The comment above from @Fishrock123 addresses confusion around "soft" and "hard" deprecate, and I like his suggestion. More clarity, less jargon.

It does leave the unaddressed the third phase (what's called end-of-life) in the doc. While end-of-life has a well-understood meaning, I think we should avoid it here as we're not quite using it the way others do, at least in my experience.

I propose that we just have two phases and indicate that the second phase ("hard" deprecation or, in @Fishrock123's suggestions, "runtime" deprecation) must last for two semver major releases. Because as far as I can tell, there's no difference between "hard" deprecation and "end-of-life" other than that "end-of-life" has to come after a major release in "hard" deprecation. So just make it two major releases and all is clear to the reader. Two labels, instead of three, less confusion, more clarity.

This all assumes I haven't overlooked something with "end-of-life" or otherwise misunderstood it.

@addaleax

This comment has been minimized.

Show comment
Hide comment
@addaleax

addaleax Aug 29, 2016

Member

I’d like to propose that we try and enumerate the possible reasons for deprecating APIs, mostly in order to check that the deprecation actually serves a purpose. Something like

APIs or parts of an API may be deprecated if the deprecation:

- Reflects security or usability issues inherent to the API, or
- Makes way for significant improvements in the underlying implementation

would be my – probably very crude – first attempt, but nearly all of the deprecations that have taken place in the past major releases fall under these categories.

As a counterexample, a deprecation of the .parent property of buffers, as it is being mentioned in #8311, would imho fulfil neither of these criteria. Supporting it as an alias for .buffer is very, very cheap, and I don’t see any reason for deprecating or working towards its removal. (I don’t want to steal the discussion from over there, but that proposal is what made me consider this.)

(edited for grammar)

Member

addaleax commented Aug 29, 2016

I’d like to propose that we try and enumerate the possible reasons for deprecating APIs, mostly in order to check that the deprecation actually serves a purpose. Something like

APIs or parts of an API may be deprecated if the deprecation:

- Reflects security or usability issues inherent to the API, or
- Makes way for significant improvements in the underlying implementation

would be my – probably very crude – first attempt, but nearly all of the deprecations that have taken place in the past major releases fall under these categories.

As a counterexample, a deprecation of the .parent property of buffers, as it is being mentioned in #8311, would imho fulfil neither of these criteria. Supporting it as an alias for .buffer is very, very cheap, and I don’t see any reason for deprecating or working towards its removal. (I don’t want to steal the discussion from over there, but that proposal is what made me consider this.)

(edited for grammar)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 5, 2017

Member

@targos and @trevnorris ... please take another look and let me know if your concerns have been met.

Member

jasnell commented Jan 5, 2017

@targos and @trevnorris ... please take another look and let me know if your concerns have been met.

@mhdawson

LGTM on these changes.

I wonder as a follow on if we need to add any words around handling changes in dependencies and how that can affect the deprecation cycle (ie we may not always be able to follow all of the steps)

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Jan 5, 2017

Member

+1 to dealing with dependency updates in a follow on pull request

Member

jasnell commented Jan 5, 2017

+1 to dealing with dependency updates in a follow on pull request

@targos

targos approved these changes Jan 5, 2017

COLLABORATOR_GUIDE.md
+- Any native C/C++ APIs/ABIs exported by the Node.js `*.h` header files that
+ are hidden behind the `NODE_WANT_INTERNALS` flag are considered to be
+ internal.
+

This comment has been minimized.

@sam-github

sam-github Jan 27, 2017

Member

Any other publically accessible part of the API, not matching an exception above, then should be considered part of the API? If so, this should be explicitly stated.

@sam-github

sam-github Jan 27, 2017

Member

Any other publically accessible part of the API, not matching an exception above, then should be considered part of the API? If so, this should be explicitly stated.

This comment has been minimized.

@sam-github

sam-github Jan 27, 2017

Member

Also, cf. #10866 (comment), we need to state more about what is the API: error message strings, throwing errors on previously ignored invalid input.

@sam-github

sam-github Jan 27, 2017

Member

Also, cf. #10866 (comment), we need to state more about what is the API: error message strings, throwing errors on previously ignored invalid input.

COLLABORATOR_GUIDE.md
+the CTC.
+
+If it is determined that a currently undocumented object, property, method,
+argument, or event *should* be documented, then a pull request adding the

This comment has been minimized.

@sam-github

sam-github Jan 27, 2017

Member

On what basis is the should decided?

@sam-github

sam-github Jan 27, 2017

Member

On what basis is the should decided?

This comment has been minimized.

@jasnell

jasnell Feb 1, 2017

Member

Generally consensus opinion

@jasnell

jasnell Feb 1, 2017

Member

Generally consensus opinion

@sam-github

The definition of the API will always leave some wiggle room, but it should be specific enough to clarify the right thing to do for, as very recent examples:

  • is the addition of properties to an options object major or minor? (#11005 (comment))
  • if tls has APIs A, and B, A is documented, but cannot be used securely without B, is B part of the API? And B doesn't fit any of the exceptions listed. Should B be documented? (#10846 (comment))
  • Using the API in a particular way achieves a useful result... but should we document it is supported, or warn that it is not? (https://github.com/nodejs/node/pull/10805/files#r97629554)
  • fs.read() with an offset doesn't modify the current read position of the file (on unix, because it uses pread(), rumour has it that it does on windows). Behaviour is part of the API, or not? (... can't recall the PR that brought this up in its comments)
    These are just the last week, we shouldn't have to debate this PR-by-PR, and users of node shouldn't have to wonder whether they can depend on a behaviour or not.

Ideally, we could say if its not documented its not part of the API, but in the absence of complete documentation, we cannot say that yet.

+ the documentation for the API must be updated to clearly indicate the
+ deprecated status.
+
+* *End-of-life* refers to APIs that have gone through Runtime Deprecation and

This comment has been minimized.

@thefourtheye

thefourtheye Jan 28, 2017

Contributor

Shouldn't we remove APIs at this level?

@thefourtheye

thefourtheye Jan 28, 2017

Contributor

Shouldn't we remove APIs at this level?

This comment has been minimized.

@jasnell

jasnell Jan 30, 2017

Member

yes, the idea is that marking an API end-of-life means that it can be removed at any time, it just may not be removed right away.

@jasnell

jasnell Jan 30, 2017

Member

yes, the idea is that marking an API end-of-life means that it can be removed at any time, it just may not be removed right away.

This comment has been minimized.

@thefourtheye

thefourtheye Jan 30, 2017

Contributor

@jasnell Once they reach this level, we will remove them from docs as well. Should we even mention level?

@thefourtheye

thefourtheye Jan 30, 2017

Contributor

@jasnell Once they reach this level, we will remove them from docs as well. Should we even mention level?

This comment has been minimized.

@jasnell

jasnell Jan 30, 2017

Member

They'll be removed from the main docs, but they will still be listed in the deprecations.md documentation page that lists the various deprecation identifiers.

@jasnell

jasnell Jan 30, 2017

Member

They'll be removed from the main docs, but they will still be listed in the deprecations.md documentation page that lists the various deprecation identifiers.

@ChALkeR

ChALkeR approved these changes Feb 1, 2017

LGTM, this is great.

+changes to a *Public* API are necessary, the existing API *must* be deprecated
+*first* and the new API either introduced in parallel or added after the next
+major Node.js version following the deprecation as a replacement for the
+deprecated API. In other words, as a general rule, existing *Public* APIs

This comment has been minimized.

@ofrobots

ofrobots Feb 1, 2017

Contributor

One challenge here is that, at present, there is no such contract between Node.js and the VMs that any VM provided API that Node.js exposes must go through the same 6mo+ deprecation cycle. Either we need to co-ordinate with VM on being able to offer this guarantee on exposed VM APIs, or we should explicitly call this out in the exceptions below.

In practice, as in the old debug protocol example recently, we have the ability to work/negotiate with the VMs to figure out the timeline – the problems is that the VMs may not necessarily know what parts of their APIs are being exposed by Node, and the kind of a deprecation guarantee Node intends to provide for these APIs that the VM needs to uphold.

@ofrobots

ofrobots Feb 1, 2017

Contributor

One challenge here is that, at present, there is no such contract between Node.js and the VMs that any VM provided API that Node.js exposes must go through the same 6mo+ deprecation cycle. Either we need to co-ordinate with VM on being able to offer this guarantee on exposed VM APIs, or we should explicitly call this out in the exceptions below.

In practice, as in the old debug protocol example recently, we have the ability to work/negotiate with the VMs to figure out the timeline – the problems is that the VMs may not necessarily know what parts of their APIs are being exposed by Node, and the kind of a deprecation guarantee Node intends to provide for these APIs that the VM needs to uphold.

This comment has been minimized.

@ofrobots

ofrobots Feb 1, 2017

Contributor

Found this comment: #7964 (comment). I'd be fine with dealing with this in a follow-on.

@ofrobots

ofrobots Feb 1, 2017

Contributor

Found this comment: #7964 (comment). I'd be fine with dealing with this in a follow-on.

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 1, 2017

Member

@sam-github @ofrobots just pushed a couple of edits that hopefully clarify a few bits.

Specifically on your comments @ofrobots ... there is an explicit statement that APIs, behaviors and errors from dependencies fall outside the scope of this policy but need to be taken into consideration when landing updates.

Member

jasnell commented Feb 1, 2017

@sam-github @ofrobots just pushed a couple of edits that hopefully clarify a few bits.

Specifically on your comments @ofrobots ... there is an explicit statement that APIs, behaviors and errors from dependencies fall outside the scope of this policy but need to be taken into consideration when landing updates.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 1, 2017

Member

Given the reviews, I believe this is ready to go ahead and land.

Member

jasnell commented Feb 1, 2017

Given the reviews, I believe this is ready to go ahead and land.

jasnell added a commit that referenced this pull request Feb 1, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: #7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 1, 2017

Member

Landed in e5bd880

Member

jasnell commented Feb 1, 2017

Landed in e5bd880

@jasnell jasnell closed this Feb 1, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 2, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: nodejs#7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@Trott Trott removed the ctc-agenda label Feb 9, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: nodejs#7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: #7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell added a commit that referenced this pull request Mar 7, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: #7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins added a commit that referenced this pull request Mar 9, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: #7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v6.10.1 proposal #11759

MylesBorins added a commit that referenced this pull request Mar 9, 2017

meta: add explicit deprecation and semver-major policy
* Formalizes deprecation policy
* Introduces End-of-life deprecation phase to identify code to be removed
* Outlines basics of internal vs. public API surface

PR-URL: #7964
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

@MylesBorins MylesBorins referenced this pull request Mar 9, 2017

Merged

v4.8.1 proposal #11760

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment