Skip to content

Coordinating HTTP2 work with Express #15203

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

Closed
benjamingr opened this issue Sep 5, 2017 · 14 comments
Closed

Coordinating HTTP2 work with Express #15203

benjamingr opened this issue Sep 5, 2017 · 14 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@benjamingr
Copy link
Member

Hey,

Related to expressjs/express#3390 - @phouri has been trying to do work in order to get Express to work with HTTP2 and it seems like there are a few questions and blockers.

I'd love to hear from @nodejs/http2 what prior work was done on getting HTTP2 to work in Express and know how we can help smooth things out.

In the thread @dougwilson wrote:

I would have been happy to point this out and help with the http2, but I didn't even realize the Node.js core http2 process was even happening until it was just about to land in Node.js 8, so I don't want to force Node.js to change everything after the fact and would rather just give in to the Node.js implementation and get the necessary changes in Express 5 to support them.


  • How should work on getting HTTP2 to work on Express (and other frameworks in the future) be coordinated between Express and core?
  • What prior art is there on this? (anything in the old http2 repo?)

I thought we could set up a short meeting to make sure everyone is on the same page and @phouri and other contributors can keep working on it.

Thanks :)

@mscdex mscdex added the http2 Issues or PRs related to the http2 subsystem. label Sep 5, 2017
@mcollina
Copy link
Member

mcollina commented Sep 5, 2017

I'm happy to jump on a meeting at any time. We really want 'http2' to be tried out by the community before unflagging from "experimental". So, the fact that express is struggling is somewhat expected - we need help to make http2 as good as it can be.

See for some of the technical details: expressjs/express#3390 (comment)

@jasnell
Copy link
Member

jasnell commented Sep 5, 2017

The whole compatibility question has been one of the key issues all along. We knew going in that there were going to be some rough spots that needed work. The compatibility API layer needs to be the main focus of this.

@apapirovski
Copy link
Contributor

Would be happy to help out in any way I can & that is needed.

@TimothyGu
Copy link
Member

Accidentally closed this, sorry!

@TimothyGu TimothyGu reopened this Sep 5, 2017
@phouri
Copy link
Contributor

phouri commented Sep 5, 2017

Needless to say I'd be more than happy to help in any way.

@benjamingr
Copy link
Member Author

benjamingr commented Sep 5, 2017

I would love to see both @phouri and @apapirovski attend the express meeting.

@apapirovski thanks a lot for all your recent HTTP2 work by the way - keep up!

@apapirovski
Copy link
Contributor

I'll try to dig into how this is all setup in hapijs/hapi (which I use in production), just to have another point of reference as we try to figure out the compatibility layer.

@apapirovski
Copy link
Contributor

apapirovski commented Sep 8, 2017

FWIW hapi seems to work with the compatibility layer. There's an issue with them maintaining a flag on the socket which becomes unavailable after the finish event but not sure if that's our or their problem (we would have to maintain a reference to the session or the socket which doesn't seem great). Hard to tell the extent to which it's compatible without running their test suite which is only h1. The site I tested it on ran fine but I'm almost certainly not using even half of the functionality available.

@grantila
Copy link

Not sure if this is the place for a for/against the compatibility layer discussion, but I'll mention my experience.

I'm building a web backend framework with a pretty different approach than the current ones (express, koa, hapi). It's still drafting/testing and not available to anyone, so there's nothing to show yet and I won't go into detail about it here. However, I never even considered touching the compatibility layer.
I just recently starting building an HTTP/2 client module (initially for unit testing the backend, but I'll publish it separately), also not considering using the compatibility layer.
So, I've been working with both the server and client parts of the new API.

In my view, HTTP/2 is radically different than HTTP/1(.1). The push is a nice feature, not that architecturally remarkable for higher-level API's like Node.js' http2 module, but the session/socket/stream handling is. It's such a big difference, I'd personally drop the compatibility layer and focus all effort and thoughts on the new one, if the manpower was mine to spend. There's currently inconsistencies in the documentation, and a few bugs and improvement issues here on Github, so I won't add to them, they cover most acute problems.

The new API, in my view, is generally good in that it's close to the protocol and does no magic. I must say though, that I'm allergic to the monoliths of the old req which is now the "stream". This having a readable which is also an event emitter, and on top of this, we just append as much features as possible - an entire http request/response - is just bad. I'd much rather have a request object where the readable/writable Node.js streams are properties. The old req object is a good example of (IMNSHO) a horrible design choice, where the readable stream should've been a property readable. I'll sleep better at night when the documentation for the http2 module, under Class: Http2Stream won't say Extends: <Duplex> . Simply because it makes zero sense for it to extend anything. We keep saying "composition over inheritance" and yet fail, repeatedly.

Overall, with the Github issues (and fixes for them) included, the API is good. One can relatively painlessly build applications (and most importantly, frameworks) on top of it. Well done, and thanks for the work.

@grantila
Copy link

A couple of minutes ago I pushed an early version of a Fetch API implementation http2-only, for Node (and its internal http2 module): fetch-h2 (npm). No compatibility layer, obviously.

@jasnell
Copy link
Member

jasnell commented Jan 8, 2018

What's the current status here? Do we need to keep this issue open?

@benjamingr
Copy link
Member Author

@jasnell unless express 5 was released with http2, I think so

@dougwilson
Copy link
Member

I've been waiting on that code from @apapirovski or at least if someone wanted to implement the same thing again. I'm happy to pick apart the code from the dump if I can get access to it 👍

@benjamingr
Copy link
Member Author

Closing as stale.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

9 participants