-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
fix: support HTTP/2 in astro dev
#11284
Conversation
🦋 Changeset detectedLatest commit: 1be0a1f The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
packages/astro/src/core/request.ts
Outdated
@@ -50,7 +50,10 @@ export function createRequest({ | |||
? undefined | |||
: headers instanceof Headers | |||
? headers | |||
: new Headers(Object.entries(headers as Record<string, any>)); | |||
: new Headers( | |||
// Filter out H2 pseudo-headers, as these can't be created with Headers |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are H2 pseudo-headers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're fake headers that are added to all HTTP/2 requests. They're only allowed to be generated internally though: it's an error to add one to Headers yourself. https://httpwg.org/specs/rfc7540.html#HttpRequest
packages/astro/test/test-utils.js
Outdated
dispatcher: new Agent({ | ||
connect: { | ||
rejectUnauthorized: false, | ||
}, | ||
allowH2: true, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this for? I have no knowledge of this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tells undici fetch to use a custom dispatcher. This lets you customise some of the HTTP options for the request. rejectUnauthorized
lets you ignore invalid server certificates, which is needed here as the dev server will always use self-signed certs. allowH2
enables HTTP/2 requests in fetch, if the server supports it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add comments for both of these
@ematipico thanks for the review. I've added comments answering both of your questions |
Changes
The Vite dev server suppports HTTP/2 when the
server.https
option is set. However this doesn't work inastro dev
by default, because we set thevite.server.proxy
option (to an empty object). If a user manually enables it by unsetting the proxy option, we generate a malformedAstro.request.url
and SSR pages break because we try to set invalid headers.This PR removes the default proxy config so it is possible to enable HTTP/2 by setting the
vite.server.https
options. It handles URL correctly now, by chedking the:authority
pseudo-header as well ashost
, and ensures that we don't attempt to create invalid headers when we copy the headers object.Fixes #10238
Testing
It adds a test fixture based on the repro in #10238 (thanks @alexvuka1). In order to do this, I also added support for TLS and HTTP/2 in our test utils. If the fixture's Astro config enables https, the test utils configure fetch to trust self-signed certs and enable HTTP/2 in our tests.
Docs
I don't think this needs to be documented