-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat: http2 support #515
base: main
Are you sure you want to change the base?
feat: http2 support #515
Conversation
@@ -87,7 +87,8 @@ export const action: ActionFunction = async ({ request, params, context }) => { | |||
} | |||
|
|||
const storage = new CacheStorage(); | |||
const contentLength = Number.parseInt(request.headers.get('Content-Length') as string); | |||
|
|||
const contentLength = Number.parseInt((request.headers.get('Content-Length') as string) ?? 0); |
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.
on http2 clients won't send the content-length header.
this will currently save the length as 0. This looks ugly in the ui, but it has no other negative effect.
it can be fixed later by updating the size after streaming the body into the store
@@ -43,6 +43,8 @@ | |||
"domain-functions": "2.5.1", | |||
"fs-blob-store": "6.0.0", | |||
"isbot": "3.7.1", | |||
"koa": "^2.15.3", | |||
"koa-static": "^5.0.0", |
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 had no succes with express, but koa supports http2 without issues
server/index.mjs
Outdated
}) | ||
); | ||
|
||
const server = http2.createServer(app.callback()); |
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.
will add an option to opt-in for http2 without tls through an env var
76f953d
to
bee2b14
Compare
Hi @macrozone, thanks for reporting the issue and investigate it to this point :) |
its not about https, its about http2. The vite plugin only adds https support for the local dev server as far as i can tell, which is usually not what you want. You usually don't want to do https yourself, since managing certificates is a pain. https is best done by the reverse proxy, in this case that would be cloud run (or e.g. nginx). Typically the reverse proxy would receive and send http2 to the outside world, do the TLS termination and then sends unencrypted http1.1 traffic to the service itself. This will lead to the 413 Payload Too Large on cloud run and potentially other platforms as well. This change here sends unencrypted http2 traffic to the service instead. my mr is a replacement of the remix-serve (which takes nearly no configuration) with a custom server, that is anyway recommended. Usually you would use express, but express does not yet support the node's native http2 server properly, so koa is the only option right now. |
Sorry for the delay, I'll see to get this merged this weekend |
Any news on this? |
on some environments, e.g. Google cloud run, there is a limit of 32MB if you use http/1.1, but some artifacts may be larger than this. in that case you will get a 413 Payload Too Large Error.
There is no limit when HTTP/2 is used.
You don't need a certificate for this, since cloud run handles the TLS termination, and then sends cleartext http2 to the service (h2c).
i used the variable USE_HTTP2_NO_TLS to set this. The name should highlight that it does not enable https