feat(context): make fetch Response headers mutable

[nitedani]

The added test fails without the PR.

Fixes:

• Return a piped response with compression enabled causes an unexpected error #3316

• Add tests

• Run tests

• bun run format:fix && bun run lint:fix to format the code

[usualoma]

Hi @nitedani, Thanks for making the pull request!

Referencing internal symbols is a very interesting implementation, but in the main body of the hono we want to ensure that it is implemented only in runtime-independent code, within the scope of the web standard.

Also, as we want to maintain high performance with simple requests, we usually do not want to include processing for ‘immutable response objects’.

About #3316, I would prefer the following approach.

diff --git a/src/context.ts b/src/context.ts
index e3dbc3c5..487e1456 100644
--- a/src/context.ts
+++ b/src/context.ts
@@ -465,16 +465,32 @@ 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)
+      try {
+        for (const [k, v] of this.#res.headers.entries()) {
+          if (k === 'content-type') {
+            continue
           }
+
+          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 {
+            _res.headers.set(k, v)
+          }
+        }
+      } 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
         } else {
-          _res.headers.set(k, v)
+          throw e
         }
       }
     }

[nitedani]

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.

[usualoma]

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.

[nitedani]

I would prefer the following approach.

That fails the added test ☹️ I think we need to check if the response headers are immutable.
Checking it is weird 😢
https://github.com/honojs/hono/pull/3318/files#diff-0cd594dddd6a9f2e3e26afebbb92716434437d7001c2416252aa7531e7f2b8d4R468-R480

Edge case: calling new Response(_res.body) throws an error if _res.body was already consumed.
For example

const res = fetch(...)
await res.text()
ctx.res = res // TypeError: Response body object should not be disturbed or locked

[usualoma]

@nitedani

Thanks for the answer. I think the test needs to be modified. I don't think ‘c.res.headers becomes mutable when c.res = res’ is a result that context.ts should guarantee. The result that should be guaranteed is that c.res = res does not result in an error'.

    c.res = res
    c.res.headers.set('X-Custom', 'Message')
    expect(c.res.headers.get('X-Custom')).toBe('Message')

What is needed is a test of the following. However, it is debatable whether it is appropriate to use the external service https://jsonplaceholder.typicode.com in unit tests here.

  it('Should be able to overwrite a fetch reponse with a new response.', async () => {
    c.res = await fetch('https://jsonplaceholder.typicode.com/todos/1')
    c.res = new Response('foo', {
      headers: {
        'X-Custom': 'Message',
      },
    })
    expect(c.res.headers.get('X-Custom')).toBe('Message')
  })

  it('Should be able to overwrite a response with a fetch response.', async () => {
    c.res = new Response('foo', {
      headers: {
        'X-Custom': 'Message',
      },
    })
    c.res = await fetch('https://jsonplaceholder.typicode.com/todos/1')
    expect(c.res.headers.get('X-Custom')).toBe('Message')
  })

[usualoma]

The following cases are problems with the code itself. It is not possible to use the res after res.text() in hono (or any other framework) as response, so I think it is not a target to be handled by context.ts.

const res = fetch(...)
await res.text()
ctx.res = res

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.77%. Comparing base (d1c7f6f) to head (00d4bf1).
Report is 29 commits behind head on next.

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #3318      +/-   ##
==========================================
- Coverage   96.31%   95.77%   -0.54%     
==========================================
  Files         151      152       +1     
  Lines       15368     9201    -6167     
  Branches     2693     2822     +129     
==========================================
- Hits        14801     8812    -5989     
+ Misses        567      389     -178     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[nitedani]

. I don't think ‘c.res.headers becomes mutable when c.res = res’ is a result that context.ts should guarantee.

I agree. Should we close the PR? Maybe we can support middleware like compression for fetch responses some other way?

[usualoma]

Hi @nitedani, Thanks for your comment.

The following branch is the one with tests added to the contents of #3318 (comment).

main...usualoma:hono:fix/update-context-res

As for #3316, I believe this is how it should be fixed.

It is up to you whether you want to reflect this in this pull request or close this pull request once and make another pull request.

[yusukebe]

Hi @nitedani @usualoma

Sorry for the delayed response. This will be awesome PR! Sometimes, I have trouble with a matter like #3316 when I fetch origin content and return it in CDN edges.

The code @usualoma wrote #3318 (comment) and #3318 (comment) looks good. Would you like us to go with it?

[yusukebe]

Hi @nitedani

Can you work on this? If you can't, I'll do it.

[nitedani]

@yusukebe Please take it.

[yusukebe]

Hi @usualoma

I've updated the @nitedani 's branch with applying your patch and pushing it. The test passed on my machine, but the CI is falling. Do you have any idea?

[usualoma]

@yusukebe
OK, mystery solved. Since we test against the branch being merged, we need to take into account the current contents of main.

With the changes in #3317, small data is no longer compressed, so the test needs to be large as well.
https://github.com/honojs/hono/blob/main/src/middleware/compress/index.ts#L36

diff --git a/runtime_tests/node/index.test.ts b/runtime_tests/node/index.test.ts
index b55512d5..310ec9c7 100644
--- a/runtime_tests/node/index.test.ts
+++ b/runtime_tests/node/index.test.ts
@@ -207,10 +207,11 @@ describe('streamSSE', () => {
 })
 
 describe('compress', async () => {
+  const cssContent = Array.from({ length: 60 }, () => 'body { color: red; }').join('\n')
   const [externalServer, externalPort] = await new Promise<[Server, number]>((resolve) => {
     const externalApp = new Hono()
     externalApp.get('/style.css', (c) =>
-      c.text('body { color: red; }', {
+      c.text(cssContent, {
         headers: {
           'Content-Type': 'text/css',
         },
 
@@ -242,6 +243,6 @@ describe('compress', async () => {
     const res = await request(server).get('/fetch/style.css')
     expect(res.status).toBe(200)
     expect(res.headers['content-encoding']).toBe('gzip')
-    expect(res.text).toBe('body { color: red; }')
+    expect(res.text).toBe(cssContent)
   })
 })

[yusukebe]

@usualoma

Cooool! It passed. Can you review this again, though it should be okay?

[usualoma]

@yusukebe Thank you, LGTM!