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

When using new Response as a return value, the headers set by c.header() are lost in the actual response. #2681

Closed
syfxlin opened this issue May 15, 2024 · 6 comments

Comments

@syfxlin
Copy link

syfxlin commented May 15, 2024

What version of Hono are you using?

4.3.7

What runtime/platform is your app running on?

Node.js

What steps can reproduce the bug?

When using new Response as a return value, the headers set by c.header() are lost in the actual response. If the response is returned using c.text, the headers will appear normally in the actual response.

image

  1. Run pnpm init
  2. Run pnpm i hono @hono/node-server
  3. Paste the following code into the index.mjs file
import { serve } from '@hono/node-server'
import { Hono } from 'hono'
import { setCookie } from "hono/cookie";

const app = new Hono()

app.get('/',async (c, next) => {
  setCookie(c, 'x-cookie-1', '1')
  c.header('x-test-1', '1')
  c.res.headers.set('x-test-2', '2')
  await next()
  setCookie(c, 'x-cookie-2', '2')
  c.header('x-test-3', '3')
  c.res.headers.set('x-test-4', '4')
}, async (c) => {
  setCookie(c, 'x-cookie-0', '0')
  c.header('x-test-0', '0')
  c.res.headers.set('x-test-0', '0')
  return new Response('Hello Node.js!')
  // return c.text('Hello Node.js!')
})

serve(app)
  1. Run node index.mjs
  2. Run curl http://localhost:3000 -v

What is the expected behavior?

The actual response should contain all the set headers.

What do you see instead?

The following is my analysis, I just using Hono for less than half a month, so the analysis may not be accurate.

Additional information

No response

@syfxlin syfxlin added the bug label May 15, 2024
@mvares
Copy link
Contributor

mvares commented May 15, 2024

@syfxlin, I saw that the second function — maybe we will call it middleware — is called first. I think it should not happen this way.

@mvares
Copy link
Contributor

mvares commented May 15, 2024

I divided the problem into two parts:

set res(_res: Response | undefined) {
  this.#isFresh = false
  if (this.#res && _res) {
    const headers = this.#res.headers

    for (const [k, v] of headers.entries()) {
      _res.headers.set(k, v)
    }

    const setCookieHeader = headers.get('set-cookie')
    if (setCookieHeader) {
      _res.headers.set('set-cookie', setCookieHeader)
    }
  }

  this.#res = _res
  this.finalized = true
}

I am waiting now that the problem is resolved 🍡

@yusukebe
Copy link
Member

Hi @syfxlin

This is not a bug. If it returns raw Response in the handler, it will be sent to the user directly. If you want to keep the header values set by c.header(), you can use c.body().

@yusukebe yusukebe removed the bug label May 15, 2024
@syfxlin
Copy link
Author

syfxlin commented May 16, 2024

Hi @yusukebe, sometimes, we may not be able to return a response using c.body (which is often the case when using some third-party middleware), but need to set certain request headers before the handler, for example, setting a cookie before it reaches the authHandler when using @hono/auth-js (Auth.js database session + credentials). Inconsistency in the behavior of c.header and c.res.headers.set can also confuse.

@yusukebe
Copy link
Member

The same issue was discussed in #1526

Some say that a context header should be added to the new Response(). But, as this comment, we don't want a design where header values are implicitly added to the Response object. If the user wants to add the header in context, they should use c.body().

Regarding @hono/auth-js, it should use c.body() instead of new Response().

@syfxlin
Copy link
Author

syfxlin commented May 16, 2024

Hi @yusukebe, thanks for your answer, I think it's acceptable not to add headers implicitly, so close this issue, it would be nice if you could state this in the documentation to avoid confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants