nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
107.96k stars 29.78k forks source link

Coordinating HTTP2 work with Express #15203

Closed benjamingr closed 6 years ago

benjamingr commented 7 years ago

Hey,

Related to https://github.com/expressjs/express/pull/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.


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 :)

mcollina commented 7 years ago

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: https://github.com/expressjs/express/pull/3390#issuecomment-327210410

jasnell commented 7 years ago

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 commented 7 years ago

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

TimothyGu commented 7 years ago

Accidentally closed this, sorry!

phouri commented 7 years ago

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

benjamingr commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 6 years ago

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

benjamingr commented 6 years ago

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

dougwilson commented 6 years ago

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 :+1:

benjamingr commented 6 years ago

Closing as stale.