Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

Why is http2 exposed as a property of the http module? #29

Closed
RobertWHurst opened this issue Dec 16, 2016 · 35 comments
Closed

Why is http2 exposed as a property of the http module? #29

RobertWHurst opened this issue Dec 16, 2016 · 35 comments

Comments

@RobertWHurst
Copy link

RobertWHurst commented Dec 16, 2016

Hi everyone. I'm just curious why http2 is exposed via require('http').HTTP2? Is this temporary, or the final API? It seems a bit clunky. I was hoping we'd see require('http2').

As an aside, this is really cool stuff. Really exciting to see this under development. Thanks for your efforts here. 🍻

@jasnell
Copy link
Member

jasnell commented Dec 16, 2016

There is an existing http2 module on npm that would conflict if we exposed the new http/2 functionality that way. It is unfortunate but necessary. And yes, hopefully it is temporary until a better solution can be found.

@sebdeckers
Copy link
Contributor

sebdeckers commented Dec 16, 2016

Could ask nicely if the author of h2 minds giving up their NPM name. Doesn't seem like there are many downloads. Also, http-2 with a hyphen is available.

@jasnell
Copy link
Member

jasnell commented Dec 16, 2016

Definite possibility. I'd rather avoid hyphenated names as much as possible. Requesting the h2 package name seems like a good choice.

@ChALkeR
Copy link
Member

ChALkeR commented Dec 16, 2016

http-2

I expect a significant number of people mistyping that as http2 and wasting time on that in the best case, and gaining some other problems from that in the worst case.

@jasnell
Copy link
Member

jasnell commented Dec 16, 2016

@jmandzik ... how would you feel about the possibility of transitioning ownership of the h2 module name to Node.js core?

@RobertWHurst
Copy link
Author

@nwgh If http2 gets added to core, would you mind giving up the namespace so the core solution can be be implemented elegantly?

@jmandzik
Copy link

@jasnell I'm happy to relinquish it, especially for node core! I'm traveling this weekend with limited access to computer, but I'll circle back with you Monday if that works.

@jasnell
Copy link
Member

jasnell commented Dec 16, 2016 via email

@RobertWHurst
Copy link
Author

RobertWHurst commented Dec 18, 2016

As much as I think h2 is better than a property tacked onto http, I think h2 might confuse users as it's not clear what h2 means. The core API should demonstrate good API design. I don't think h2 for http2 exactly fits in this regard as it's inconsistent with the http and https module. Would love to hear what @nwgh thinks here.

@nwgh
Copy link

nwgh commented Dec 19, 2016

Hi all. Sorry for the lag, must be that time of year :) Anyway, I'm more than happy to give up the "http2" namespace for core. I definitely think that makes the most sense in all respects. I don't have great access to rename the package on npm right now, but later this week I'll rename to something (once I come up with a reasonable name) as I personally (in my role as a Firefox developer) will continue to depend on what we currently know as "http2", as we use that for integration testing of our http/2 stack.

I also agree with @RobertWHurst that "h2" is a little un-descriptive. While it matches the ALPN token (and is what I normally use with other networking types), it's too easily confused with the h2 HTML tag. Best to be more explicit, in my opinion.

I'll circle back around once I've done the rename to let everyone know the namespace is free.

@mcollina
Copy link
Member

@nwgh I don't know what is the process for all of this. We should probably get NPM and the TSC/CTC involved. I would like to keep the ecosystem disruption as small as possible.

@jasnell
Copy link
Member

jasnell commented Dec 19, 2016

We should definitely work through the process with npm... paging @zkat and @isaacs for assistance on this.

@zkat
Copy link
Contributor

zkat commented Dec 19, 2016

You shouldn't really need us for this. https://www.npmjs.com/policies/disputes is our name dispute policy, and it has instructions on transferring names (tl;dr npm owner add <new-owner>). Keep in mind that formerly-published versions cannot be published over, and if they've been out long enough, cannot be unpublished. This should not be an issue, as you can simply deprecate it with a message to use the native node@8 one, but even users on older nodes will see it.

Personally, I'd just add a note in the README that http2 is superseded by the native module if you're in node@>=8. You could even add an engines entry so that it warns only on that.

If you want more guidance than this or something actually official (I am not saying this stuff in any official capacity. I'm simply mentioning what options you have), then please email [email protected] so we can handle this with existing processes. I don't see any issues, tho :)

@isaacs
Copy link
Contributor

isaacs commented Dec 19, 2016

What @zkat said also happens to be the official position of npm, Inc. on this matter.

If node-core adds a new builtin that takes over a name currently used in npm, well, that sucks. We've been there before. Please be very careful about that.

If you want the existing owner to move off the name, follow this: https://www.npmjs.com/policies/disputes

If you want technical guidance about how to make that go smoothly, I'm happy to help out.

@jasnell
Copy link
Member

jasnell commented Dec 19, 2016

@zkat ... Thank you very much for the clarification. That's what I had suspected but wasn't sure if the unpublish policy changes had altered the process on this at all.

@zkat
Copy link
Contributor

zkat commented Dec 20, 2016

I figured it would be good for other folks here if I spelled out the technical constraints here.

How Do Native Module Conflicts Work?

  • Native modules always take precedence over node_modules.
  • If users on node@>=8 do npm install http2, they will only be able to require('./node_modules/http2') to use the installed one.
  • npm install http2 will be a complete NOOP for users, except for the fact that npm scripts such as postinstall will still run. This is a security risk if node core does not have control over the package, since malicious actors could take advantage of unwitting/uninformed users. It's possible for a dependency to pull this stunt as well, but that's a general vulnerability with public registries to begin with anyway.
  • Users will never find out that their http2 version installed from the registry is not being used at all, unless there's a deprecation message telling them as such.

Possible Steps

This is an idea of one way you could do it. Feel free to mix and match and reword depending on what you feel is best:

  1. Grab the current ([email protected]) source code and CD into it.
  2. Update README.md telling users to use http2@legacy if they need compatibility with node@<8, and explain the overall situation with package transition.
  3. npm deprecate http2 'Update to node@8 or install http2@legacy to continue using this module.' -> deprecations only apply to already-published versions, not entire packages.
  4. put "engines": { "node": "<8" } in package.json
  5. npm version patch -> [email protected]
  6. echo 'tag=legacy' >> .npmrc -> Make publishes default to legacy instead of latest.
  7. npm publish -> latest: 3.3.6; legacy: 3.3.7
  8. Change to "engines": { "node": ">=8" }
  9. npm version major -> [email protected]
  10. npm publish --tag latest -> latest: 4.0.0; legacy: 3.3.7
  11. npm deprecate http2@4 'This module has been superseded by the native module in node@8. Use http2@legacy on node versions under 8'

Results

This is what various kinds of users will see, depending on their current setups:

  • npm-shrinkwrap.json: Will not see a deprecation warning until they manually update.
  • npm install on node@8 with http2 already in package.json: Will see an engines warning.
  • npm install on node@7 with http2 already in package.json: No message.
  • npm install http2 on node@8: Will see engines warning AND http2@4 deprecation warning. Will error if they try to se it.
  • npm install http2 on node@7: Will see http2@4 deprecation warning, instructing them to use http2@legacy. Will error if they try to use it.
  • npm install http2@legacy on node@8: Will see engines warning. It will still install.
  • npm install http2@legacy on node@7: No engines or deprecation warnings.

Alternatives

You could simplify all of the above by just publishing a patch version where http2 has a failing install script that does exit 1 if the current node version is node@8, telling users to install http2 as an optionalDependency. This would be an alternative path to provide compatibility.

Alternatively, you could literally deprecate the entire http2 module, publish a new patch version with an added engines field and a crashing index.js to drive the point home, and re-publish the package under a different name, instructing users to use that package instead if they want to use this code. This approach would make npm.im/http2 available for node@8 users who want an alternative implementation, and would like to continue maintaining that module for LTS users, separately.

The simplest path is to just... not do anything. Make sure there's no way to get malicious scripts. You could deprecate the module altogether once node@<8 all go out of maintenance, just to make sure people stop installing the damn module. Because it won't do anything.

@RobertWHurst
Copy link
Author

RobertWHurst commented Dec 20, 2016

I know I'm not involved in the heavy lifting going on here, but I want to say; @nwgh and @jmandzik, It's very cool of you guys to offer up your namespaces. I could imagine building a popular package then passing it off to the core team can feel a bit difficult. Super cool to see you guys so willing to do so in order to make node better. Love seeing this kind of kindness 🍻

@mcollina
Copy link
Member

I do not want to remove enthusiasm from everyone, but http2 in core is not ready to ship, and the current plan is to land it in node 8 behind an experimental flag, or with a "experimental" stability indicator: http2 support will break while we go, and there will be bugs.
For the whole of node 8 life, the better option will be to use the 'http2' module on npm if anyone wants stability. I hope this can exit the experimental phase in node 9.

We have plenty of time to come up with the best plan that minimize disruption, and to support a long transition time. As an example, we could rename http2 on npm before node v8, and ship http2 in core behind a flag. In node v9, we could drop the flag.

@not-an-aardvark
Copy link
Contributor

Would it be possible to have the 2 as a module path, i.e. require('http/2')? This would avoid package name conflicts (http is already a core module), and it's also very recognizable (HTTP/2 is actually the official name of the spec).

@RobertWHurst
Copy link
Author

RobertWHurst commented Jan 17, 2017

@not-an-aardvark Why? We already have agreement from @nwgh, the package owner of http2 for using it's package name once this feature eventually drops.

@not-an-aardvark
Copy link
Contributor

Given that the existing http2 module has over 100000 downloads per month, changing the meaning of require('http2') to return a different module will probably cause a significant amount of breakage. We could change it in a major version, of course, but unless people consider the name http/2 to be significantly worse than http2, it seems like it would be better to pick a name that avoids breakage altogether.

@RobertWHurst
Copy link
Author

RobertWHurst commented Jan 18, 2017

Just my two cents here, but I believe it's better to be consistent with the rest of the core modules. http/2 implies to me that it's a sub component for the http module which it is not.

As for rolling the major. It can't be done as core modules are not versioned in the user's package.json. As for code breakage, I would argue that when a user upgrades their node version a whole major, they are opting into breaking changes and will need to fix old code. This is the same as any major release of node where there are breaking changes. Adding HTTP2 should be a whole release major in itself.

@not-an-aardvark
Copy link
Contributor

As for code breakage, I would argue that when a user upgrades their node version a whole major, they are opting into breaking changes and will need to fix old code.

The user might not be depending on http themselves -- they might have a dependency three levels deep which is using the http2 module. Upgrading in situations like this is often nontrivial. It's typically better to avoid breaking changes unless there's a good reason to do so, even in a major release.

I'm not saying that we can't use the http2 name -- I'm just saying that we should consider non-breaking alternatives such as http/2 before we jump to the decision that a breaking change is the only way to go.

@RobertWHurst
Copy link
Author

RobertWHurst commented Jan 18, 2017

@not-an-aardvark I agree, minimizing breakage is very important. I would just add that so is consistency. Without it you end up with an ugly system. http/2 seems incorrect. A / in a module name has been a separator of a parent and child namespace. 2 is not a module name, and http is not it's parent.

The course of action I think would be best here is that the http2 module gets republished under a new name, for example 'http-2' or something else. The original http2 module gets updated with a depreciation warning indicating users to switch to http-2. Then months later when Node is released with HTTP2 in core, the bulk of the users of the original http2 NPM module will have switched over to the new http-2 module, and the http2 namespace will be available.

Based on my suggestion above I think the things we would need to know here are:

  • When is node with http2 likely to ship?
  • How many months before node ships do we need to update the http2 module with a depreciation warning?
  • What new namespace can we use for @nwgh's handy work to keep it available?

@not-an-aardvark
Copy link
Contributor

For what it's worth, I think http/2 could be reasonably consistent if we made http/1 an alias for http. http would effectively be a namespace for all the core http libraries (and it would scale well if there is ever an http3, for example).

@ronkorving
Copy link
Contributor

ronkorving commented Jan 18, 2017

I'm probably gonna sound like a complete tool (since I haven't really followed the API design work being done here), but can't http2 simply be married with the already existing built-in http and https? I'm sure that would be a challenge, but would it be impossible? Both spdy and http2 modules on NPM have an API that is in essence very similar to the built-in http module, they "just" add extra API on top of that.

@jasnell
Copy link
Member

jasnell commented Jan 18, 2017

The http/2 protocol is fundamentally different from http/1 is so many ways that somehow merging the two would be entirely impractical and, in many cases, impossible.

@isaacs
Copy link
Contributor

isaacs commented Jan 18, 2017

This really is a bikeshed. Here's what I'd recommend (because of course what this is lacking is Yet Another opinion, ha ha)

  1. Include HTTP/2 support in core as require("http").http2, as it currently is. As warts go, it's not a bad one.
  2. Send a PR to the http2 module to do this: var http = require('http'); if (http.http2) return module.exports = http.http2; // else, API-matching userland implementation. It might be a breaking change. That's fine, just bump the major version.
  3. Leave it that way until everyone is using a version of http2 that just exports the core module, and a version of node that includes http.http2. At that time, move the core module to require('http2') and the http2 author can npm deprecate http2 "HTTP/2 support is included in Node.js core now, this module is no longer necessary. People who really need to use the userland one can do require('http2/'). It may take a few years to get to that point. We at npm, Inc. are happy to provide y'all with version usage numbers and download counts.
  4. Never ever remove require('http').http2. Leave that there forever. It harms no one, and removing it would be unnecessary user-hostility.

@RobertWHurst
Copy link
Author

RobertWHurst commented Jan 19, 2017

@isaacs I see what you're saying here. I'm just unclear why you suggest going the warty road here. Unlike the browser environment node has proper versioning. The trade off here in my opinion is that node ends up looking rather confusing to newcomers. It doesn't seem unreasonable to ask that a node user update their code when they decide to bump the Node version a major release.

@isaacs
Copy link
Contributor

isaacs commented Jan 19, 2017

@RobertWHurst It's easy to underestimate the cost of breaking changes in Node.js, because as you say, upgrades are opt-in, so what's a few more changes?

But these changes add up, and they result in creating the impression that Node.js is unstable, unreliable, and chasing newness for its own sake rather than investing in quality and the needs of users. You can argue it's unfair, but the fairness doesn't change the perception.

We want people to upgrade. We should make it as easy as possible to do so, even if that means a slightly more cluttered API surface. And really, require('http').http2 is fine.

@akc42
Copy link

akc42 commented Jan 19, 2017

Why should it be require('http').http2 and not require('https').http2 Also should not http2.createServer() with the same parameters as the https version be how you create a server?

@jasnell
Copy link
Member

jasnell commented Jan 19, 2017

At this point, we do not need to nail down what the exact namespace / entry point will be. We will have a reasonable entry point and we will take ecosystem impact into consideration in the process. I really appreciate the current owners of the h2 and http2 modules being willing to turn those over if necessary but it's too early to say for certain that we'll need them. When the time comes to finalize on that decision, the CTC will reach out and work through whatever issues there may be.

@Somebi
Copy link

Somebi commented Feb 3, 2017

So after all, how it will be called? Any docs available? Is it available in nightly build? Any info related to http2 should be added to the main page. It's kinda missing or i wasn't reading carefully...

@jasnell
Copy link
Member

jasnell commented Feb 3, 2017 via email

@jasnell
Copy link
Member

jasnell commented May 5, 2017

Closing this as we're going to be going with require('http2') for the time being.

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

No branches or pull requests