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: update experimental status to reflect use #12723

Closed
wants to merge 3 commits into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Apr 28, 2017

  • Update the experimental status to reflect actual common use.
  • Also make a few formatting fixes since I'm in here.

Fixes: #12701

Checklist
Affected core subsystem(s)

doc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Apr 28, 2017
@Fishrock123
Copy link
Contributor

Fishrock123 commented Apr 28, 2017

I'm not exactly OK with locking ourselves into experimental features...

This is exactly what we moved away from, what we had and worked out poorly in the past.

This feature is still under active development and subject to non-backwards
compatible changes, or even removal, in any future version. Use of the feature
is not recommended in production environments. The feature may only be usable
only after using a command-line flag to enable it. Use of the feature may cause
Copy link
Contributor

Choose a reason for hiding this comment

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

double only?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

```

```txt
Stability: 1 - Experimental
This feature is subject to change, and is gated by a command line flag.
It may change or be removed in future versions.
This feature is still under active development and subject to non-backwards
Copy link
Contributor

Choose a reason for hiding this comment

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

Here you use this, while everywhere else you use the

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is consistent with the existing language. For instance, under deprecated, it starts with 'This feature' and has the feature in the next sentence.

@refack
Copy link
Contributor

refack commented Apr 29, 2017

I'm not exactly OK with locking ourselves into experimental features...

@Fishrock123 May I ask in what sense are we locking ourselves? Do you feel that defining experimental is futile (users will complain anyway)? or is it something in the phrasing?

This is exactly what we moved away from, what we had and worked out poorly in the past.

Could you share an example? (honest question!)

Correct me if I'm wrong, but as I see it the alternatives are:

  1. Not releasing anything until it's rock solid
  2. Not documenting these "experimentals" at all

@gibfahn
Copy link
Member

gibfahn commented Apr 29, 2017

May I ask in what sense are we locking ourselves?

The change here is from "experimental means behind a flag" to "experimental might mean behind a flag". I assume @Fishrock123 was referring to discussion in #12701.

@gibfahn
Copy link
Member

gibfahn commented Apr 29, 2017

FWIW I'm -1 on this. Once URL becomes stable, I don't think we have anything that is experimental but not behind a flag. When the next experimental is added, I'd rather the default was "it should be behind a flag", for the reasons @sam-github covered pretty exhaustively in #12701 (comment).

@jasnell
Copy link
Member Author

jasnell commented Apr 29, 2017

Async_hooks is still experimental and not behind a flag.

@jasnell
Copy link
Member Author

jasnell commented Apr 29, 2017

@Fishrock123 ... It's not clear at all what you mean by "locking ourselves in to experimental features"

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

IMHO the wording is now clearer, and more consistent with the status quo

Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, also Fixes: #11362
(edit: imho, there’s one more: also Fixes: #9036)

@jasnell
Copy link
Member Author

jasnell commented May 1, 2017

@nodejs/ctc ... PTAL

@ChALkeR
Copy link
Member

ChALkeR commented May 1, 2017

On «Experimental»

Um. It's not great that we introduce APIs that are exprected to change and are not behind the flag.
The whole idea of the «experimental» is to message that changes should be expected with a higher probability than in regular API.

Given how the ecosystem actually uses the API, we had problems even changing undocumented API (and got yelled at for that). I don't think that introducing more unflagged API that is expected to change will improve the situation here. How about we just flag everything new until it's ready, like Chromium/V8 does?

I suppose that we need a discussion about this.

On «Deprecated»

The new description in fact contradicts with the recent decision about the old Buffer API. It is currently deprecated in a sense that a complete drop-in replacement exists and that the old API usage is highly discouraged, but we recently decided on a CTC vote that we do not plan any changes there at this point. The description in this PR directly contradicts that.

ChALkeR
ChALkeR previously requested changes May 1, 2017
Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

See explanation in the comment above.

@addaleax
Copy link
Member

addaleax commented May 1, 2017

Given how the ecosystem actually uses the API, we had problems even changing undocumented API (and got yelled at for that).

That’s because those APIs were the opposite of experimental, though; they had been around for so long that they basically had to be considered part of the public API, whereas experimental APIs are quite fresh (usually too young to have a significant share of the ecosystem come to rely on them, flagged or unflagged).

How about we just flag everything new until it's ready, like Chromium/V8 does?

I think I’d be -1 on doing that unconditionally, Node’s ecosystem has a different way of adopting new features than JavaScript itself.

I agree with what you say about “Deprecated”, though.

@jasnell
Copy link
Member Author

jasnell commented May 1, 2017

I've seen absolutely no evidence to support the position that explicitly opt-in APIs must be put behind a flag. Recent experience has shown exactly the opposite. If we look at recent history with experimental features that (a) were not behind a flag and (b) had no documented issues of causing any problems (for instance, WHATWG URL and async-hooks), I think that it should be fairly clear that a flag is not strictly necessary if we're quite clear on the documentation and user expectations side of things.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax addaleax mentioned this pull request May 2, 2017
4 tasks
@AndreasMadsen
Copy link
Member

Regarding async-hooks:

The async-hooks API doesn't exist in the master branch, the PR is still open. Its predecessor async-wrap does however exist. The Diagnostic WG calls the async-wrap API pre-experimental since it is purposely kept undocumented, can only be accessed using process.binding magic, API breaking changes have been made in a patch update, and we tell everybody not to use it.

It would truly surprise me if there are more than 10 people in the world that use async-wrap directly, thus it is not a good candidate for measuring "documented issues".

@ChALkeR
Copy link
Member

ChALkeR commented May 4, 2017

Note: my -1 is mostly for the Deprecated documentation change, as that contradicts with a recent CTC decision (that is still applicable at the current state of this PR).

For changing the definition of Experimental, I'm mostly -0 now — I would want that part to gain more consents from the CTC members, but I'm not strictly against that. /cc @nodejs/ctc

@ChALkeR ChALkeR dismissed their stale review May 4, 2017 14:21

There are no actual changes in the Deprecated description.

@ChALkeR
Copy link
Member

ChALkeR commented May 4, 2017

Sorry, it looks like I made a mistake — Deprecated description did not receive any actual changes here, that is mostly formatting.

I have dismissed my review.

I will open a new issue for the problem with the Deprecated description.

That said, I would want to see more acknowledgement from @nodejs/ctc members on the Experimental description change (upd: you could count this as -0).

@ChALkeR
Copy link
Member

ChALkeR commented May 4, 2017

Another note: this or #12710 fixes #11362, whichever lands first.

Copy link
Contributor

@MylesBorins MylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM with tiny nit

It may change or be removed in future versions.
This feature is still under active development and subject to non-backwards
compatible changes, or even removal, in any future version. Use of the feature
is not recommended in production environments. The feature may only be usable
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
s/The feature may only be usable The feature may only be usable/The feature may require the use of a command-line flag to enable it

@sam-github
Copy link
Contributor

@chrisdickinson PTAL, this removes text you added in #943

@isaacs You experienced API lock-in, you may have comments on this and/or #12701

@sam-github
Copy link
Contributor

I still don't agree with this. There is a long history of js code depending on undocumented node properties, APIs, and behaviours and Node.js being locked into inability to make changes we want to, I agree with #12723 (comment)

While I agree that that the URL and async-wrap were not a source of problems, I think they have some very unique characteristics, and don't convince me that "the times have changed".

URL implements an API that was pre-specified and much discussed by the WhatWG before it was implemented in Node, which makes it a very atypical "experimental" API (I think its actually unique in Node, we have no other APIs externally specified).

async-wrap, #12723 (comment), was a fringe API, not likely to be widely used, and arguably not very useful for its target users (thus its successor, async-hooks). Also, as far as I know, we have not deleted it yet, so we may yet hear screaming (though I don't expect we will).

@jasnell
Copy link
Member Author

jasnell commented May 7, 2017

@sam-github ... ok, I'll think through this a bit more. That said, one notable experimental feature that we're going to have coming up soon is http/2. That is entirely new code that would have zero impact on existing code and it's use is completely opt-in. Other than the potential for people using it before it's done (and therefore having to put up with breaking changes), there is, in theory, no technical reason it would need to be behind a flag -- we may want to do so, but there's no detrimental technical impact if we do not do so. With that, there are two approaches:

  1. We explicitly require opt-in to using it by requiring a flag (which is what we're doing with N-API)
  2. We emit a process warning if it happens to be used.

Option one is stricter, option two is far more lenient. I guess the choice, then, comes down to how much we want to try to protect users against themselves :-)

@jasnell
Copy link
Member Author

jasnell commented May 11, 2017

Updated the text per discussion. PTAL

@Fishrock123
Copy link
Contributor

As stated in #9036, which @jasnell agreed with at the time, I think we should print warnings for all experimental features.

In addition, i think we need to reserve, by default, the ability to change or remove experimental APIs.

I think we can consider, outside of the default to do something different if need be for our users, but I think it should be assumed to be able to change at any time until marked as Stable.

@sam-github
Copy link
Contributor

This came up in the last @nodejs/diagnostics WG meeting, nodejs/diagnostics#94

@ofrobots and @watson stated that there products would not use an API if using caused warnings to be emitted and used by users, which I agree with, strong-agent would not have done that either, any warnings that user's see are considered bugs by users, so not acceptable in production code.

But I think this calls into question the purpose of "experimental". In my opinion, this is somewhere where we should be more similar to Chrome.

It can be difficult to get feedback on APIs while they still exist on PR branches, its a fairly small group of users who are willing to compile Chrome or Node in order to try out a feature.

We land these features on master (and then Current) so that users can experiment with them, but in Chrome, you still have to turn on experimental features in chrome://flags/, the node equivalent is a CLI (and we expose CLI options via NODE_OPTIONS, so they can also be enabled via environment).

The purpose is to make it simultaneously:

  1. reasonably easy for users to experiment with and give early feedback on APIs
  2. reserve the freedom to change the APIs at any time until they are stable

I don't have a crystal ball. Its possible that async hooks, or the js inspector API (#12263) will land as "docs only" experimental, and either we don't find need to make backwards incompatible changes to them, or that not enough people are using them to notice when we do change them.

Maybe people will treat the docs seriously, I'm willing to give it a try, but I think its risky, I'd rather hide experimental features behind flags, like Chrome does, and then advertise them widely via twitter/node roundup/etc. so we can get feedback on them, and work on getting them stable quickly (and backported to LTS if at all possible).

@jasnell
Copy link
Member Author

jasnell commented May 12, 2017

@ofrobots and @watson stated that there products would not use an API if using caused warnings to be emitted and used by users

This is rather the point. We do not want these going into production environments and if emitting a warning provides enough discouragement to do so, then a flag is not necessary.

@jkrems
Copy link
Contributor

jkrems commented May 12, 2017

We do not want these going into production environments and if emitting a warning provides enough discouragement to do so, then a flag is not necessary.

One problem is that they "won't be able" to use those features once the warning is disabled either. Because it would be hard or even impossible to test if the feature still generates a warning. They would need to guard everything with node version checks because the existence of a function or module wouldn't mean that it can be used w/o noisy warnings.

@sam-github
Copy link
Contributor

Its nice to have simple testing of existence of a feature, but we already do semver comparisons on node's version, how we monkey-patch http.get depends on the version (since http.get doesn't call http.request anymore as it used to).

@jasnell
Copy link
Member Author

jasnell commented Jul 24, 2017

Landing given the sign off...

jasnell added a commit that referenced this pull request Jul 24, 2017
* Update the experimental status to reflect actual common use.
* Also make a few formatting fixes.

Fixes: #12701

PR-URL: #12723
Fixes: #12701
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Jul 24, 2017

Landed in 5c2d1af

@jasnell jasnell closed this Jul 24, 2017
addaleax pushed a commit that referenced this pull request Jul 24, 2017
* Update the experimental status to reflect actual common use.
* Also make a few formatting fixes.

Fixes: #12701

PR-URL: #12723
Fixes: #12701
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@Fishrock123
Copy link
Contributor

I think this is a big step backwards, but ¯\_(ツ)_/¯ I guess.

@addaleax addaleax mentioned this pull request Aug 2, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* Update the experimental status to reflect actual common use.
* Also make a few formatting fixes.

Fixes: #12701

PR-URL: #12723
Fixes: #12701
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
* Update the experimental status to reflect actual common use.
* Also make a few formatting fixes.

Fixes: #12701

PR-URL: #12723
Fixes: #12701
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 3, 2017
* Update the experimental status to reflect actual common use.
* Also make a few formatting fixes.

Fixes: #12701

PR-URL: #12723
Fixes: #12701
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
* Update the experimental status to reflect actual common use.
* Also make a few formatting fixes.

Fixes: #12701

PR-URL: #12723
Fixes: #12701
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confirmation that the stability definition is correct