feat(adapter-node): add http2 support #185
Conversation
| "editor.formatOnSave": true, | ||
| "[typescript]": { | ||
| "editor.defaultFormatter": "esbenp.prettier-vscode" | ||
| }, |
There was a problem hiding this comment.
Added this out of convenience.
| type GenericResponse = { | ||
| write(chunk: Uint8Array): boolean; | ||
| once(event: "drain", listener: () => void): void; | ||
| once(event: "error", listener: (err: unknown) => void): void; | ||
| off(event: "drain", listener: () => void): void; | ||
| off(event: "error", listener: (err: unknown) => void): void; | ||
| }; | ||
|
|
||
| async function writeAndAwait( | ||
| chunk: Uint8Array, | ||
| res: ServerResponse, | ||
| res: GenericResponse, |
There was a problem hiding this comment.
Needed because write methods of http.ServerResponse and http2.Http2ServerResponse are not compatible according to the type-checker, even though they are when called without a callback.
There was a problem hiding this comment.
This file is used by functions that want to support both node:http and node:http2. For example, createRequestAdapter and sendResponse use these types.
|
Looks great! |
| export interface DecoratedRequest extends Omit<IncomingMessage, "socket"> { | ||
| ip?: string; | ||
| protocol?: string; | ||
| socket: PossiblyEncryptedSocket; |
There was a problem hiding this comment.
This property was non-optional, but I saw another DecoratedRequest type in src/request.ts had it set as optional. Which one is right?
There was a problem hiding this comment.
It's not optional, req.socket is always defined.
What I'm trying to achieve is that there's a badly documented req.socket.encrypted property that only exists when using node:https. The adapter uses it to infer the protocol (https or http) part of the URL when origin isn't manually set and trustProxy is false. (req.protocol is a non-standard Express property, useful when embedding a Hattip app into an Express app as a middleware).
I think the one in src/request.ts is just an error on my part.
aleclarson
force-pushed
the
feat/adapter-node-http2
branch
2 times, most recently
from
ca02ba9 to
f22ea93
Compare
|
The test will fail since Node only supports HTTP/2 over TLS. Also, it appears that native fetch in Node does not support HTTP/2 yet? nodejs/undici#2750 I don't know when I'll be able to investigate a solution. |
|
I'll start changing the testing setup to use |
|
As far as I can understand Node can do HTTP/2 without SSL (just added tests for that, it seems to work). What it can't do seems to be upgrading from non-secure HTTP 1.1 to HTTP/2, it only supports upgrades via TLS ALPN. More info here: nodejs/node#44887 |
aleclarson
force-pushed
the
feat/adapter-node-http2
branch
from
f22ea93 to
cee461b
Compare
|
@cyco130 Just rebased this PR. We're seeing timeouts in the Could be my own fault, as I opted to keep the HTTP2 test fixture I wrote, since getting the new |
Added support for
node:http2. I decided not to create a newadapter-node-http2package, since they share a lot of code.Same exact API as
@hattip/adapter-nodeHTTP/1.1 API.Other entry points include:
@hattip/adapter-node/http2/native-fetch@hattip/adapter-node/http2/whatwg-nodeNo tests have been added yet.