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

feat(context): make fetch Response headers mutable #3318

Merged
merged 8 commits into from
Sep 8, 2024
Merged
Prev Previous commit
Next Next commit
immutability check
  • Loading branch information
nitedani committed Aug 25, 2024
commit 40230839bd93766038f2b763ed981e667deac6f8
48 changes: 26 additions & 22 deletions src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -464,34 +464,38 @@ export class Context<
*/
set res(_res: Response | undefined) {
this.#isFresh = false
if (this.#res && _res) {
this.#res.headers.delete('content-type')
for (const [k, v] of this.#res.headers.entries()) {
if (k === 'set-cookie') {
const cookies = this.#res.headers.getSetCookie()
_res.headers.delete('set-cookie')
for (const cookie of cookies) {
_res.headers.append('set-cookie', cookie)
if (_res) {
try {
// Check if `_res` headers are immutable.
_res.headers.delete('______________________________')
} catch (e) {
if (e instanceof TypeError && e.message.includes('immutable')) {
// `_res` is immutable (probably a response from a fetch API), so retry with a new response.
this.res = new Response(_res.body, {
headers: _res.headers,
status: _res.status,
})
return
}
}

if (this.#res) {
for (const [k, v] of this.#res.headers.entries()) {
if (k === 'set-cookie') {
const cookies = this.#res.headers.getSetCookie()
_res.headers.delete('set-cookie')
for (const cookie of cookies) {
_res.headers.append('set-cookie', cookie)
}
} else if (!_res.headers.has(k)) {
Copy link
Contributor Author

@nitedani nitedani Aug 25, 2024

Choose a reason for hiding this comment

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

Why wasn't this checked before? It feels unintended to overwrite the fresh headers with the old ones. Anyways, this would be a breaking change so I'll keep it out of the PR.

Copy link
Member

Choose a reason for hiding this comment

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

I think that point is correct. I think the new response headers should be prioritised. And you're right, it would be a spec change and should be discussed in another PR.

// Prioritize the new response's headers over the old response's headers.
_res.headers.set(k, v)
}
} else {
_res.headers.set(k, v)
}
}
}
this.#res = _res
this.finalized = true

// fixes: https://github.com/honojs/hono/issues/3316
if (this.#res) {
const headersObj = this.#res.headers as unknown as Record<symbol, string>
const guardSymbol = Object.getOwnPropertySymbols(headersObj).find(
(symbol) => symbol.toString() === 'Symbol(guard)'
)
// Make the headers mutable again
if (guardSymbol && headersObj[guardSymbol] === 'immutable') {
headersObj[guardSymbol] = 'response'
}
}
}

/**
Expand Down