Changing Definition of "Locked" for Stability Index in API Docs #11200
Comments
MylesBorins
added
meta
|
Looks good to me, with |
|
I have mixed feelings. I think the last sentence seems to contradict the first two sentences (with an emphasis on seems). I also wonder if the new sentence is more directed at ourselves than a typical contributor. Like, instead of "It can be argued that this is already covered by bug fixes", I would say that is in fact the plain reading of the text. I think the whole "will reject attempts to accommodate actual language features" was a result of us not adhering to that text and instead trying to use Locked as this strange we-get-to-have-it-both-ways "no, we're not deprecating this, but it's not for you and you shouldn't use it" statement. Maybe the actual Locked text can be left alone in all its wonderful brevity (and in fact, if anything, it could be made more brief by simply removing the second sentence as it is redundant) and we can instead add this additional information as explanatory text that isn't part of the ends-up-in-a-bannerish-box-in-the-docs text: ```txt Stability: 3 - Locked Only bug fixes, security fixes, and performance improvements will be accepted. ``` Changes to support new language features that are added to the ECMA-262 specification are considered bug fixes and are therefore permitted in Locked APIs. EDIT: That would be in |
|
I like Rich's suggestion. |
|
I'm not that comfortable classifying all changes to support new language features as bug fixes. Some of them are flat out API changes. For instance, if TC-39 adds a new method to |
|
@jasnell suggested:
Remove "handled independently of this stability index" (does not impart any useful information, just raises questions and muddies the otherwise clear meaning) and you've got something good, I think: |
|
+1 although I'd go with "may be considered" rather than "are permitted" |
|
Not sure about this (the part that limits the allowed changes to ecma262-induced). It also doesn't seem to cover other changes that landed to «Locked» modules not so long ago, e.g.:
Note: the list above is outdated, there are probably newer ones. |
I don't think there's any language that limits anything to ecma262-related stuff, or at least that's not my intention. It's supposed to explicitly allow ecma262-related stuff, but not necessarily limit it to just that. Perhaps that's an argument for leaving it out entirely and just going with: ```txt Stability: 3 - Locked Only bug fixes, security fixes, and performance improvements will be accepted. ``` We decided at the CTC meeting that things like "make assert.deepStrictEqual() work as expected with Maps and Sets" could be considered a bug fix (on a case-by-case basis) but we don't necessarily have to get into that level of detail in our docs about the Stability Index. |
What I am talking about is that That is either a bug in the process of accepting patches to those three modules (modules, timers, assert) or a bug in the documentation of that process. If the former, then no such pull requests should have been accepted at the first place, and shouldn't be accepted in the future. If the latter, the stability index documentation needs a more severe change that covers the way how we have been actually dealing with accepting patches to those modules in a clear way. Now the actual process breaks users expectations, as it significantly differs from what is documented — we can't keep saying that those modules accept only bugfixes and keep accepting semver-major and semver-minor level changes there, we have to choose something. |
|
I personally think that with #7964 in place, we could move away from the «Locked» concept entirely (re-label those three modules as And that would match with how we have been actually dealing with patches to those modules. |
IMO, it's the former.
I don't think that's the plan.
Works for me if I'm wrong and we do plan on accepting semver-minor and semver-major on currently Locked APIs. |
@Trott so you're of the opinion that things like Given the definition of I'm not sure what Or are we saying that we won't add new APIs either? If so what is the rationale behind that? Not breaking things seems somewhat orthogonal to adding new APIs. EDIT: It seems
@Trott Does locked mean kinda-deprecated? I just thought it meant even-more-stable. |
I'm under the impression Locked is supposed to preclude new methods, so yes, that is my opinion.
For I'm all for simplifying. If the consensus is that |
|
I like the idea of less stability levels. In the distant past, the intention of experimental was, I believe, to say that the feature could change in the next release (at that time, a minor jump from 0.x to 0.x+1). But that would imply that semver-major changes happen to experimental, but AFAICT, we treated the "experimental" APIs with as much care as the "stable" APIs, because apps depended on them. It looks like some attempt was made to retroactively define "experimental" as meaning "you have to turn it on with a command line option, and we really do feel free to change this in the next major, you've been warned", see https://nodejs.org/api/documentation.html#documentation_stability_index, which seems more protective. But these experimental APIs uses don't match that definition:
The fourth current use is https://nodejs.org/api/all.html#debugger_v8_inspector_integration_for_node_js, which is actually hidden behind a flag, so fits definition of experimental. There are no other uses, leaving it pretty close to droppable. Locked was intended to discourage feature requests. I don't know if it has done that, and it is only used for timers, assert, and module. It might be better to rename them to stable, and just add a note in the docs about why we aren't interested in feature additions, or even a node FAQ or pre-canned response, which allows us to add any API that we, the people writing nodejs unit tests, would find useful without getting yelled at, but still reject requests to make it better for non-node core unit testing. |
|
Also, note that two of the experimentals are tracking upstream deps over which we have no control, and that we will follow whatever they do (v8 debug protocol and WhatWG URL spec). Also, the inspect behaviour is a flag, so whether that counts as being "behind a flag" (as the experimental stability requires) or not is debatable. I think we should delete both experimental and locked, in case my conclusion wasn't clear. |
|
As a first step, I'm going to see if there's general support for making If that seems to work for people, we can talk about But if we don't have the will to do it for |
Change the Stability Index on `assert` from Locked to Stable. Refs: nodejs#11200
|
@sam-github I am not sure about the WHATWG URL implementation For general usage it's pretty spec-complaint at this point AFAICT, minus some edge cases, but then browsers are not 100% spec-complaint either, so it doesn't need a flag to discourage broader usage IMO. |
|
@joyeecheung I don't have any comment on the stability of the WhatWG spec, maybe they do allow breaking changes. However,
By that definition, url.URL cannot be regarded as "Experimental". The other options are Locked, Deprecated, and Stable. I don't think its Locked, and its definitely not Deprecated, so it must be Stable. Or the definition of "Experimental" is wrong, take your pick! |
|
Sorry, I was a little bit vague, I mean the URL implementation is sort of between "Experimental" and "Stable". The API is not controlled by Node.js and it doesn't fit the description of "Stable"(not "proven satisfactory" and still accepts "nice-to-have" but not "absolutely necessary" edge-case breaking changes), it is experimental at this point AFAICT(perf and spec compliance still has work to do) but in practice it's not behind a flag. I think it's about whether we should let the stability index reflect our practices or the other way around. |
|
Also I think URL still fits in the description of Experimental if the "gated by command line flag" part is loosen up a little? And this won't be a question if we want to leave the experimental status in Node 8? (Not very sure about the timeline though, maybe other people from @nodejs/url has more clue than I do) |
|
Except that historically, introducing an API means it cannot be changed without breaking code, and people scream when their code breaks, the fact that having said we reserve that right doesn't make them feel better. The putting it behind a flag means that it is actually impossible for a node module to depend on this feature unless the ultimate consumer of that module specifically passes a flag to node when they invoke it, which is a fairly strong indicator of willingness to live on the edge. It means both the author of the js code depending on the experimental feature, and the consumer of the js code explicitly indicate their willingness to use experimental code. Otherwise, its easy for someone to accidentally use an API and not notice its experimental, and its almost impossible for a consumer of a module to know that happened. In the absence of the passing of a command line flag, I don't think we should be breaking our APIs - everything should be treated as 'stable', and we should go through a deprecation/warning cycle if we want to introduce a breaking change. |
Change the Stability Index on timers from Locked to Stable. Note that this is not intended to encourage changes to the timers API, but to allow it when its useful for Node.js (as has happened in violation of the documented stability level), and possibly to simplify the stability levels by removing Locked altogether. PR-URL: nodejs#11580 Ref: nodejs#11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Update documentation for `module` to reflect stability index 2 (Stable) rather than 3 (Locked). Refs: nodejs#11200
The stability index 3 (Locked) is unused and is being eliminated. Remove it from the documentation about the stability index. Remove mention of the Locked from CONTRIBUTING.md. The remaining text about the stability index is slight and not seemingly valuable. Removing it too. Refs: nodejs#11200
|
PR to unlock module and remove Locked from the stability index: #11661 |
Change the Stability Index on timers from Locked to Stable. Note that this is not intended to encourage changes to the timers API, but to allow it when its useful for Node.js (as has happened in violation of the documented stability level), and possibly to simplify the stability levels by removing Locked altogether. PR-URL: #11580 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Update documentation for `module` to reflect stability index 2 (Stable) rather than 3 (Locked). PR-URL: nodejs#11661 Ref: nodejs#11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
The stability index 3 (Locked) is unused and is being eliminated. Remove it from the documentation about the stability index. Remove mention of the Locked from CONTRIBUTING.md. The remaining text about the stability index is slight and not seemingly valuable. Removing it too. PR-URL: nodejs#11661 Ref: nodejs#11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
|
Locked is removed in 51cea05. Closing. |
Update documentation for `module` to reflect stability index 2 (Stable) rather than 3 (Locked). PR-URL: #11661 Ref: #11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
The stability index 3 (Locked) is unused and is being eliminated. Remove it from the documentation about the stability index. Remove mention of the Locked from CONTRIBUTING.md. The remaining text about the stability index is slight and not seemingly valuable. Removing it too. PR-URL: #11661 Ref: #11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on `assert` from Locked to Stable. PR-URL: #11304 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on `assert` from Locked to Stable. PR-URL: #11304 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on `assert` from Locked to Stable. PR-URL: #11304 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on `assert` from Locked to Stable. PR-URL: #11304 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Evan Lucas <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on timers from Locked to Stable. Note that this is not intended to encourage changes to the timers API, but to allow it when its useful for Node.js (as has happened in violation of the documented stability level), and possibly to simplify the stability levels by removing Locked altogether. PR-URL: #11580 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Update documentation for `module` to reflect stability index 2 (Stable) rather than 3 (Locked). PR-URL: #11661 Ref: #11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
The stability index 3 (Locked) is unused and is being eliminated. Remove it from the documentation about the stability index. Remove mention of the Locked from CONTRIBUTING.md. The remaining text about the stability index is slight and not seemingly valuable. Removing it too. PR-URL: #11661 Ref: #11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on timers from Locked to Stable. Note that this is not intended to encourage changes to the timers API, but to allow it when its useful for Node.js (as has happened in violation of the documented stability level), and possibly to simplify the stability levels by removing Locked altogether. PR-URL: #11580 Ref: #11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Update documentation for `module` to reflect stability index 2 (Stable) rather than 3 (Locked). PR-URL: #11661 Ref: #11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
The stability index 3 (Locked) is unused and is being eliminated. Remove it from the documentation about the stability index. Remove mention of the Locked from CONTRIBUTING.md. The remaining text about the stability index is slight and not seemingly valuable. Removing it too. PR-URL: #11661 Ref: #11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
Change the Stability Index on timers from Locked to Stable. Note that this is not intended to encourage changes to the timers API, but to allow it when its useful for Node.js (as has happened in violation of the documented stability level), and possibly to simplify the stability levels by removing Locked altogether. PR-URL: nodejs/node#11580 Ref: nodejs/node#11200 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>
Update documentation for `module` to reflect stability index 2 (Stable) rather than 3 (Locked). PR-URL: nodejs/node#11661 Ref: nodejs/node#11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
The stability index 3 (Locked) is unused and is being eliminated. Remove it from the documentation about the stability index. Remove mention of the Locked from CONTRIBUTING.md. The remaining text about the stability index is slight and not seemingly valuable. Removing it too. PR-URL: nodejs/node#11661 Ref: nodejs/node#11200 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Сковорода Никита Андреевич <[email protected]> Reviewed-By: James M Snell <[email protected]>
As discussed in last weeks @nodejs/ctc meeting
Currently our api docs have a stability index with fairly conservative definition of
LockedThis has been fairly useful when discussing changes to APIs that are potentially breaking, such as any change to the module system. With changes coming to the ecma262 standard on a yearly basis now, we find ourselves in a slightly different situation then when this was originally drafted. As the language changes we may be required to change locked APIs, such as assert or module, in order to accommodate the changes to the language.
It can be argued that this is already covered by
bug fixes, but that is somewhat ambiguous, and it would be better to be explicit about itA suggested alternative text
Thoughts?
The text was updated successfully, but these errors were encountered: