-
Notifications
You must be signed in to change notification settings - Fork 209
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
[Miniflare 3] Allow custom services to respond with Content-Encoding
and multiple Set-Cookie
headers
#613
Conversation
Previously, if the response from a custom service included a `Content-Encoding` header, we would respond with decoded content to `workerd` without removing the `Content-Encoding` header. This caused `workerd` to try to decode already decoded content, resulting in an error. This was particularly a problem with `wrangler pages dev`, as upstream dev servers could very easily return gzipped content. This change updates the `writeResponse()` function with code from Miniflare 2, to re-encode the body before responding. Note, due to a fixed but unreleased bug in `undici` (nodejs/undici#2159), defining a custom service that directly proxies somewhere else with `fetch()` like `wrangler pages dev` will fail, if `Content-Encoding` contains multiple encodings. Closes cloudflare/workers-sdk#3408
Previously, if a custom service returned a response with multiple `Set-Cookie` headers, they would be combined into a single header. This change uses the `set-cookie-parser` package to split the header, ensuring `Headers#getSetCookie()`/ `Headers#getAll("Set-Cookie")` return all cookies in a Worker.
|
script: `export default { | ||
async fetch(request, env, ctx) { | ||
const res = await env.CUSTOM.fetch(request); | ||
return Response.json(res.headers.getSetCookie()); |
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.
TIL getSetCookie 🤯
Why was Ubuntu latest cancelled? |
@petebacondarwin the Ubuntu 16.13.0 tests have been particularly flakey, and sometimes the test runner times out without actually terminating the job. We're yet to figure out what's causing this. Rather than burning actions minutes, the job was cancelled and retried. |
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 see this is merged. I was wondering how the test for the _transformsForContentEncoding()
works. Are you just using that to setup the encodings but then checking that the actual writeResponse()
code does the correct thing? In other words, does this test actually check the writeResponse()
code?
Previously, if the response from a custom service included a
Content-Encoding
header, we would respond with decoded content toworkerd
without removing theContent-Encoding
header. This causedworkerd
to try to decode already decoded content, resulting in an error. This was particularly a problem withwrangler pages dev
, as upstream dev servers could very easily return gzipped content.This change updates the
writeResponse()
function with code from Miniflare 2, to re-encode the body before responding.Closes cloudflare/workers-sdk#3408
Previously, if a custom service returned a response with multiple
Set-Cookie
headers, they would be combined into a single header.This change uses the
set-cookie-parser
package to split the header, ensuringHeaders#getSetCookie()
/Headers#getAll("Set-Cookie")
return all cookies in a Worker.