Skip to content
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 custom servers using HTTP/2 in production #12989

Merged
merged 8 commits into from
Nov 13, 2024
Merged

Conversation

benmccann
Copy link
Member

closes #11365

Copy link

changeset-bot bot commented Nov 11, 2024

🦋 Changeset detected

Latest commit: dcefdcf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

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

Copy link
Member

@Conduitry Conduitry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In #3572, we were able to check the major version of the incoming request and only operate on it then. Is that a possibility here, to avoid doing extra work cloning objects and looking for properties on every incoming request?

@benmccann
Copy link
Member Author

done

I hadn't remembered that we had this before 😆 guess we should have kept it rather than merging #7142. perhaps undici has gotten stricter than it used to be

@eltigerchino
Copy link
Member

This was something I tried in the previous PR but ran into some issues with the request URL being set incorrectly during https (#12907 (comment)). Can you verify if this works with HTTPS turned on during dev / preview while navigating around a SvelteKit website?

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-kit-12989-svelte.vercel.app/

this is an automated message

@benmccann
Copy link
Member Author

It should work now, but please test and let me know if it doesn't

@benmccann
Copy link
Member Author

If we merge #12977 first it might help with testing

@eltigerchino
Copy link
Member

eltigerchino commented Nov 13, 2024

LGTM. Managed to reproduce the issue I had earlier where the host was undefined during vite preview with HTTPS. To fix it, I replicated the HTTP2 host assignment logic from the vite/dev/index.js code to vite/preview/index.js.

req.headers[':authority'] || req.headers.host

const host = req.headers['host'];

@benmccann
Copy link
Member Author

Awesome. Thanks for the help!

@benmccann benmccann merged commit 0bd4426 into main Nov 13, 2024
14 checks passed
@benmccann benmccann deleted the psuedo-headers branch November 13, 2024 04:14
@github-actions github-actions bot mentioned this pull request Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

undici's new Request is incompatible with HTTP/2 requests
4 participants