avoid creating unnecessary headers instance#1008
avoid creating unnecessary headers instance#1008anonrig wants to merge 1 commit intoopennextjs:mainfrom
Conversation
🦋 Changeset detectedLatest commit: a955067 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
commit: |
conico974
left a comment
conico974
left a comment
There was a problem hiding this comment.
We need to check, I might be wrong, but in some cases we can pass the result of a fetch directly here, and in this case the headers are immutable.
| for (const cookie of cookies) { | ||
| responseHeaders.append("Set-Cookie", cookie); | ||
| } | ||
| headers["set-cookie"] = cookies.join(","); |
There was a problem hiding this comment.
I think that's wrong.
I you have a=1,b=2 it will create a single a cookie with the value 1,b=2
There was a problem hiding this comment.
> let a = new Headers()
undefined
> a.append('set-cookie', 1)
undefined
> a.append('set-cookie', 2)
undefined
> a
Headers { 'set-cookie': '1, 2' }
This is true. Other than the missing extra space
There was a problem hiding this comment.
| headers["set-cookie"] = cookies.join(","); | |
| headers["set-cookie"] = cookies.join(", "); |
There was a problem hiding this comment.
@anonrig Looks like the actual syntax would be: a="1";b="2" (quotes required)
There was a problem hiding this comment.
I see. Let me fix it.
anonrig
force-pushed
the
yagiz/avoid-creating-unnecessary-headers-instance
branch
from
ad2c46b to
a955067
Compare
| // Build headers array - set-cookie must be added separately for each cookie | ||
| const headerEntries: [string, string][] = Object.entries(headers); | ||
| for (const cookie of cookies) { | ||
| headerEntries.push(["set-cookie", cookie]); |
There was a problem hiding this comment.
Hmm.. while this might be more efficient I'm not convinced this is right. This is going to end up skipping any kind of validation on the value of cookie, which could end up causing problems elsewhere.
| // See https://github.com/cloudflare/workers-sdk/issues/8004 | ||
| if (url.hostname === "localhost") { | ||
| responseHeaders.set("Content-Encoding", "identity"); | ||
| headers["content-encoding"] = "identity"; |
There was a problem hiding this comment.
this now mutates the headers object coming from outside the function, which changes its behavior (it now has a side effect on the caller values).
5 participants
Let's make sure that we don't copy all of the headers, just to mutate it.
Part of #1002