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

Revisit putting new features behind flags #369

Closed
MylesBorins opened this issue Sep 27, 2017 · 21 comments
Closed

Revisit putting new features behind flags #369

MylesBorins opened this issue Sep 27, 2017 · 21 comments

Comments

@MylesBorins
Copy link
Contributor

With both N-API + HTTP2 putting new features behind a flag has been a barrier to adoption. This is especially true as we approach LTS and are potentially locking ourselves to multiple years of supporting APIs behind flags... meaning that it is potentially 3 years before we will see mass ecosystem adoption.

I would like to propose that we revisit our policy around landing new features behind flags

@sam-github
Copy link
Contributor

I don't understand, its supposed to be a barrier to adoption, we don't want people adopting experimental features without opting in to them.

If the features are stable enough that we are willing to treat any changes to their API as semver-major, then they should be promoted to stable and the barrier removed. Or just land the features from day 1 as stable.

If we don't want the flag, we should remove the experimental status, because it doesn't mean anything. I'm OK with this. If we don't want to land APIs that are WIP, so that people can experiment with them and give feedback (relatively) easily, allowing us to adjust/break the API before it is adopted and any breaking change would be semver major --- then lets get rid of experimental.

As for LTS, removing a the flag barrier is semver-minor. We can do it in a LTS branch.

If we remove the http2 flag now, we are committing to 3 years of supporting the http/2 API, as it is now. I've no opinion on this. If people familiar with it believe its stable, then lets take the flag off.

@sam-github
Copy link
Contributor

our policy around landing new features behind flags

Btw, the above is not our policy, to my knowledge. We put features behind flags when we want the freedom to make breaking API changes within a semver-major release line. Both HTTP/2 and napi were like this, we wanted to be able to change them, so they were behind a flag, not because they are new (perf_hooks is another example, its new, but not behind a flag).

@ofrobots
Copy link
Contributor

ofrobots commented Sep 27, 2017

From a pragmatic perspective, having NAPI behind a flag has meant that we have gotten zero to little experimentation from the ecosystem.

We currently have two mechanism to state that something is experimental:

  • Documenting something as experimental. This gives us the freedom to make breaking API changes within a semver-major release line.
  • Additionally putting it behind a runtime flag. I am not sure what additional value this provides.

IMO, the purpose of experimental is to get feedback from the ecosystem on a new feature with the ability to make breaking changes based on the feedback. For both NAPI and http2 the runtime flag has meant that very few modules are willing to experiment and provide feedback. It defeats the purpose of experimental.

@sam-github
Copy link
Contributor

sam-github commented Sep 27, 2017

Documenting something as experimental. This gives us the freedom to make breaking API changes within a semver-major release line.

That does not describe past experience. Maybe developers have changed in the last few years, but node had doc-only experimental APIs for years, and they could not be changed without causing eco-system breakage.

Particularly harsh is that it was not easily possible to know if one of your dependant modules was using an experimental API, so its not even possible to tell a user "its your fault for using the API", because it wasn't them.

I have no problem with landing things more aggressively as stable, but I think we're kidding ourselves if we think that we can make breaking changes to node APIs just because we put a doc note at the top reserving that right.

I'm puzzled by how you expect people to both use the experimental features, and yet not publish them to npmjs.org, and somehow instantly adapt to breaking changes so as to not cause widespread downstream breakage. If its some fringe package that breaks, I guess no one will notice, but I think we all remember leftpad.

Then again, I'm OK with trying it as an experiment, pun intended, lets see how loud the screams are when the breaking changes are made. And if they aren't loud, we gambled and won, I guess.

@ofrobots Given that chrome makes the exact opposite decision as you are advocating for node, I'm curious why you think node is so different from Chrome? Chrome forces experimental features to be explicitly turned on, and it also rejects extensions that use them from the Web Store, something we don't have the ability to do with npmjs.org. The problems I assume you are trying to avoid using these barriers aren't problems you think will effect the node ecosystem?

https://developer.chrome.com/extensions/experimental

@targos
Copy link
Member

targos commented Sep 28, 2017

I think to make a decision we need to understand why in some cases having a feature behind a flag blocks adoption. It's not always the case. A big counter example would be the koa ecosystem which was built on features behind the --harmony-generators flag.

@mcollina
Copy link
Member

I would like to note that http2 got enough early adopters that are enabling it to move out of experimental sooner rather than later. We have been landing multiple PR every week and the Express community is helping out by adding support. The problem for http2 is only related to the npm module, and the fact that obscuring a module from npm is a semver-major change.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 28, 2017

This is just speculation, but napi could have seen slower adoption because these types of modules live deeper in a user's node_modules directory. If a napi module is a small building block for other modules, then you have to convince a lot more people to enable the feature, including other modules. With something like http2 or generators with koa, the application developer has to turn the feature on, but not as many intermediate developers have to.

@sam-github
Copy link
Contributor

I agree with @cjihrig. In addition, if you look at https://nodejs.org/api/n-api.html, you'll see there doesn't seem to be any advice (at least, I didn't find it just now) on how to actually use it in practice.

n-api doesn't exist on older versions of node, and while I'm pretty sure there is a npm module that allows n-api to be used with older nodes, there isn't any info there on that module, so using n-api means we need both to use a command line flag, and lose support for LTS node versions.

People are willing to work hard for a new feature like HTTP/2, even using a newer node and command line flags, but the capability to write node addons already exists, so while the benefits of ABI independence and a simpler to use addon API are real and will get better with time, at this point, the loss of support (or lack of docs?) on how to use n-api with older node LTS lines was probably the real barrier, not the command line flag.

@ofrobots
Copy link
Contributor

ofrobots commented Sep 28, 2017

@sam-github

I have no problem with landing things more aggressively as stable, but I think we're kidding ourselves if we think that we can make breaking changes to node APIs just because we put a doc note at the top reserving that right.

This issue is not about changing the pace at which we move things from experimental to stable. It is specifically about the merits of putting experimental things behind flags in addition to marking them experimental. Let's keep the discussion focussed on that.

As it stands, whether or not we put new, substantial, features is behind a flag seems arbitrary.

  1. async-hooks was added in Node 4 as experimental without any flags. This has allowed experimentation in the community, and we have been able to get a lot of feedback and as a result improve async-hooks substantially (including making breaking changes). Not having a flag was the correct decision.
  2. napi was added with a flag. This has actively hampered module authors from being able to publish experimental versions of their modules (as @cjihrig explains) and get feedback from their downstream users. This in turn has limited our ability to get feedback going into an LTS. Putting this behind a flag was the wrong decision in the first place.
  3. The new inspector API was added without a flag. As far as I can see, there is meaningful experimentation in the ecosystem. I do think core retains the ability to the make breaking changes without backlash because the API is still experimental, and module authors understand this. Not having a flag was the correct decision.
  4. HTTP2 was added with a flag. We got some feedback from the ecosystem despite the flag. From a Google Cloud perspective, the only reason we able to do experimentation (and gather feedback) was the understanding that flag will eventually get removed in time for LTS. We cannot publish an experimental module building on top of http2 if we need to convince downstream module authors and users. @mcollina points out that the issue is the conflict with the ecosystem module named http2, which is fair. However, the flag allowed a (debatably) semver major implementation details to hide until a much later point. It would have been a lot better for these issues to be apparent at the time a feature is being added as a 'semver minor'. Putting this behind a flag has provided zero value.
  5. vm.runInDebugContext has been experimental for a very long time with no flags. We are just about to make a breaking change with impunity, without adhering even to the deprecation policy. This is patent proof that flags are not necessary to give us (node core) the ability to make breaking changes to experimental features.

As for the comparison to Chrome; the nature of changes is very different. --harmony flags enable language features. Code that was a syntax error before now suddenly becomes valid code. The changes we are talking about in Node code require module authors to explicitly require a specific module in order to opt into the experimental feature. Apples to Oranges.

We, as the node project, need to have a policy around when flags should be used, rather than being arbitrary about it. I argue that there are very few cases (maybe none?) where flags for new modules have been beneficial to the project.

@sam-github
Copy link
Contributor

Again, @ofrobots we don't have a policy of putting new substantial features behind flags, we have a policy of putting experimental features behind flags, this isn't at all the same thing.

  1. async hooks: did we make breaking changes to async hooks?
  2. napi: I believe that the lack of adoption is related to it not having a documented way to support older LTS versions, so use loses more than it gains, which is not effected at all by the the CLI flag
  3. inspector was added with a flag, you needed --inspect. Also, this was a new feature, but it was misclassified as experimental, IMO. The inspect protocol and the chrome debugger behind it was stable. Its categorization as experimental was unnecessary.
  4. http2: have breaking changes been made? Also, its been pointed out that people have been using http/2, so its a counter example.
  5. vm.runInDebugContext predated the "use a flag" policy, I believe, and I would say we have had many conversations about what do about our breaking it, and how to make sure there is an equivalent functionality people can use instead, which is not impunity. Its also barely used. At this point, I suspect @bnoordhuis regrets adding it, though it seemed a good idea at the time.

The purpose of marking things experimental is to allow us to make breaking changes. If the history shows we do not make breaking changes --- or when we do, we do it because we have no other choice, such as for V8 changes -- then we should remove the experimental stage and go directly to stable. It will simplify our maintenance policy and documentation.

The changes we are talking about in Node code require module authors to explicitly require a specific module in order to opt into the experimental feature. Apples to Oranges.

I have to explicitly install a Chrome extension that uses an experimental API, but I also have to enable those APIs. Sounds pretty apples to apples to me, but this is getting off track.

If I understand you, @ofrobots , your experience is that you want to use experimental features in production, and are willing to take the risk, and make whatever changes you need to make to follow along with node, but you can't because you don't control the node users CLI and can't enable the features.

I respect this, its my position with the APM modules I've worked on that willingly and knowingly depend on internal or experimental APIs and implementation details. If they change, we won't complain, we just update the code, we are willing to take the risk and will closely track the APIs we use and adapt to any changes. I think we are in the minority.

Some of the APIs above are used only by developers like us (runInDebugContext, async hooks), so we track them closely, and users of the APIs have been willing to accept changes.

I don't think HTTP/2 (as an example) is in that category, its the kind of thing that could become widely depended on by general users, not just active contributors like yourself who are willing to pay the price if the API changes.

@ofrobots
Copy link
Contributor

we don't have a policy of putting new substantial features behind flags, we have a policy of putting experimental features behind flags

Where is this policy documented?

async hooks: did we make breaking changes to async hooks?

Quite a few actually. Remember that It was originally named async_wrap.

napi: I believe that the lack of adoption is related to it not having a documented way to support older LTS versions, so use loses more than it gains, which is not effected at all by the the CLI flag

The discussion at the last VM summit specifically pointed out
that the flag is a barrier to experimentation.

inspector was added with a flag, you needed --inspect.

I meant require('inspector') API. This doesn't require a flag; is experimental; and we could make breaking changes if we needed to.

http2

This was added as a semver minor. It turns out that this is actually semver major – just that the flag hides the semver major impact. Not having the flag would have brought this forward much earlier on.

At this point, I suspect @bnoordhuis regrets adding it, though it seemed a good idea at the time.

Regardless of who added the API, it has been an extremely useful enabling in process debugging. There were no alternatives to achieve this functionality. It has cost us nothing to have this. I personally do not regret that Node had this API. We have something better now, so it is okay to remove it.

If I understand you, @ofrobots , your experience is that you want to use experimental features in production

I want to be able to write experimental modules and then get real world feedback. Flags prevent that.

The gist argument here seems to be 'Node must have the ability to make breaking changes to experimental features, and flags are necessary for that'. My arguments are

  1. This is not established policy.
  2. The latter is not required for the former.
  3. It would be nice to have some user and ecosystem empathy and not focus solely on what node core needs.

@ljharb
Copy link
Member

ljharb commented Sep 28, 2017

There is another perspective that ecosystem empathy would involve not allowing it to be polluted with modules using experimental features in the first place.

@mcollina
Copy link
Member

@ofrobots

http2
This was added as a semver minor. It turns out that this is actually semver major – just that the flag hides the semver major impact. Not having the flag would have brought this forward much earlier on.

This was discussed at length in nodejs/http2#29. Moreover in the PR that was opened nodejs/node#14239 the fact that we will be obscuring an ecosystem module is noted in the PR. @jasnell writes "...which is the main reason for gating this behind a flag". This was duly noted there, so saying that this was not brought forward early on is not correct. We didn't write that it would have been a semver-major change to drop the flag, but I would say it should be expected when we planned to drop the flag? At least that is what I understood.

It's very easy to expose the above through require('http/2') or require('http').v2, if we think it's a better idea and we can move http2 out of the experimental status. It was a trivial change back then, and It's a trivial change now.

Can we move something out of the experimental status during the LTS lifecycle? I don't think http2 is ready just know, it might be in a month or maybe in three.

@sam-github

http2: have breaking changes been made? Also, its been pointed out that people have been using http/2, so its a counter example.

Breaking change to the existing http2 module in npm? Definitely, it's a brand new implementation done with a completely different approach. So, it does not classify as breaking to me.


Regarding the current topic of the issue, which is that putting things behind a flag lowers adoption and feedback, I disagree. http2 got the feedback it needed. The core feature of n-api over nan is that it is a stable ABI, having something that changes every node version does not provide a good base for experimentation.

Moreover, n-api did not get the feedback it needed because it was extremely hard for a developer to move from nan to n-api. I did some experimentation and even in July the process was very hard if you did not know how n-api worked. Some critical pieces had very little documentation (or even unit tests) at that point. Tools for doing prebuilds (a must-have feature for all native module authors) are still completely missing for n-api. As a developer with some native modules it still does not offer a good alternative to nan: it took years for the nan ecosystem to mature with all the tools that it needs. It's ok and it will take some amount of time for the ecosystem to mature for nan. Things changed heavily since then, and n-api is almost ready to go, but it is still missing some of the tools it needs to shine.

@sam-github
Copy link
Contributor

@ofrobots

Current policy is:

https://nodejs.org/api/documentation.html#documentation_stability_index

It was https://github.com/nodejs/node/blob/71f22c842bc3f9e04ebf110461c42b07a304f352/doc/api/documentation.md not long ago, and it has been historically contentious. Obviously still is.

@MylesBorins

I would like to propose that we revisit our policy around landing new features behind flags

I'm unaware of anything written that says we have such a policy.

@ofrobots

It would be nice to have some user and ecosystem empathy and not focus solely on what node core needs.

Encouraging fragility in the ecosystem by making it easy for node users to inadvertently become dependent on experimental features is not something that I would call empathy.

Its not at all clear to me what kind of breaking changes you are willing to accept. The Experimental policy as stated would seem to allow breaking changes in LTS branches, even, since it states:

This feature is still under active development and subject to non-backwards compatible changes, or even removal, in any future version.

Any future version includes patch releases in LTS release lines.

I as well would hope that our policy would be more clearly stated, whatever it is, see nodejs/node#12701, so I look forward to some clarification on this, and it would be interesting to hear from other users what their experience has been writh breaking changes in experimental APIs.

I'm generally a fan of experimentation in policy. If we want to make it easier for users to depend on APIs we make no stability guarantees for it we will find out soon enough if that causes pain to our users, so lets try.

Perhaps @ofrobots you can propose text for a new policy.

@jasnell
Copy link
Member

jasnell commented Sep 28, 2017

I would like to propose that we revisit our policy around landing new features behind flags

There is no policy, there is only convention that is applied on a case-by-case basis.

For N-API, I would argue that there likely was not a really great reason to keep it behind a flag given that it's presence in the code has no impact on existing code. The only reason it was put behind a flag is that some argued that use should be explicitly opt in.

For HTTP/2, the reasons were much more clear. Without the --expose-http2 flag, the PR to land HTTP/2 would have been semver-major so http/2 would only have been able to exist at all from 9.x on. With the flag, we were able to get http2 into the hands of users early in 8.x as a semver-minor. So in this particular case, the use of the --expose-http2 flag actually helped drive early adoption and feedback and has helped us be able to advance it further along the road of not being experimental.

We've always known that the existing http2 module on npm would be problematic. This is a module that has been downloaded over 160k times in the past month. Landing the core http2 support with the --expose-http2 flag was motivated purely out of empathy for those existing users, in order to give them an opportunity to transition. We did this with the knowledge and cooperation of that module's primary maintainer.

It simply cannot be reasonably argued that --expose-http2 has held the http2 module back.

That said, it was never expected for the --expose-http2 flag to always be required, and my hope is that we will be able to switch it off. The http2 module was written the way it was specifically to allow it be a semver-minor addition to core, and even the commit that landed in master that switches off the --expose-http2 flag keeps the flag but makes it a non-op. Again, driven by empathy for anyone who has been using it.

The next step will be to open a PR that removes the requirement for the flag in 8.x along with an option for turning http2 off, effectively flipping the default. I see absolutely no reason why that cannot be done in LTS.

In fact, I plan on opening that PR against 8.x within the next day or so (I'll likely be working on it on my plane ride back to CA tonight).

This is not something that I think we need a written policy for. We have been handling this case-by-case. For any given case, there needs to be a good reason to add a command line flag, and there needs to be a good reason not to add a command line flag. And if there is disagreement about it, then we resolve it using our regular consensus seeking model.

It is possible to have too many policies.

@mhdawson
Copy link
Member

On question of can we bring something out of experimental in LTS, I'm hoping the answer is yes in the case of N-API. If its ready then I don't see it as much different than back porting a sermver minor to an LTS release.

@Fishrock123
Copy link
Contributor

With both N-API + HTTP2 putting new features behind a flag has been a barrier to adoption

I would like to point out that this is literally the point. We don't want "those" features to gain wide immediate adoption so that we have a runway in which we can receive some actual-use feedback while still being able to make sweeping changes (or even removal) if necessary.

@ofrobots
Copy link
Contributor

@Fishrock123 I agree with you. We don't want wide adoption, but we don't have to make it difficult for the ecosystem to be able to effectively experiment and provide feedback.

As a module author, I don't see any tangible benefits of a new feature being behind a flag. I understand what experimental entails, and I am willing to use experimental features and provide feedback to make node better. Flags make this hard.

@jasnell

For HTTP/2, the reasons were much more clear. Without the --expose-http2 flag, the PR to land HTTP/2 would have been semver-major
The next step will be to open a PR that removes the requirement for the flag in 8.x

These statements contradict each other. If there is a path to add http2 as a semver minor, could we not have followed it originally?

http2 is a compelling enough feature that we were able to get feedback despite the flag. There is no evidence that I see that the module ecosystem has benefitted from http2 being behind a flag.

I guess my main concern has been around the unwritten mantra that new features go behind flags. If the agreement is to evaluate each on a case-by-case basis, rather than being a requirement, that is fine by me. It is just that I don't yet see a case where the flag benefits the ecosystem.

Experimental APIs are experimental, flag or none.

@jasnell
Copy link
Member

jasnell commented Sep 29, 2017

These statements contradict each other.

Not really. Keep in mind that the only reason we are saying this is semver-major is because of the existing ecosystem module. For core itself, it's actually semver-minor. Further, is an experimental feature, which we typically don't even apply semver rules to in the first place! The point is, we are treating this as a semver-major only as a courtesy to give the ecosystem module users a bit of time. The owner of the module has already deprecated the module, and we've done plenty of messaging around this. Dropping the flag should be fine.

On top of that, we can remove the flag while also adding an environment variable that switches the core http2 module back off, so if someone really can't move off the ecosystem module in 8.x, then they would have a simple way of doing so that doesn't carry the same burden for us as a command line flag.

We didn't go with this approach initially because it wasn't necessary to do so. This gives us a solid path forward while still being empathetic to existing users... And you get http2 in 8.x without a flag.

@Fishrock123
Copy link
Contributor

I'm unsure where the line exactly should be here, but an easy cut off might be for entirely new modules, with other features case-by-case, particularly those with a "large" api surface.

@MylesBorins
Copy link
Contributor Author

Closing as I think we have adequately discussed this

Please feel free to reopen

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants