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

Content-type is not specified when returning c.text in bun #2284

Closed
watany-dev opened this issue Feb 28, 2024 · 4 comments · Fixed by #2666
Closed

Content-type is not specified when returning c.text in bun #2284

watany-dev opened this issue Feb 28, 2024 · 4 comments · Fixed by #2666
Labels

Comments

@watany-dev
Copy link
Contributor

watany-dev commented Feb 28, 2024

What version of Hono are you using?

4.0.7

What runtime/platform is your app running on?

Bun

What steps can reproduce the bug?

import { Hono } from 'hono'

const app = new Hono()

app.get('/', (c) => {
  return c.text('Hello Hono!')
})
app.get('/text', (c) => c.text('Text Response'))
app.get('/html', (c) => c.html('<p>HTML Response</p>'))
app.get('/json', (c) => c.json({ message: 'JSON Response' }))
app.get('/no-content-type', (c) => {
  return c.redirect('no content')
})

export default app
curl -I localhost:3000/json
HTTP/1.1 200 OK
Content-Type: application/json; charset=UTF-8
Date: Wed, 28 Feb 2024 08:40:43 GMT
Content-Length: 0

curl -I localhost:3000/text
HTTP/1.1 200 OK
Date: Wed, 28 Feb 2024 08:41:05 GMT
Content-Length: 0

curl -I localhost:3000/html
HTTP/1.1 200 OK
Content-Type: text/html; charset=UTF-8
Date: Wed, 28 Feb 2024 08:41:09 GMT
Content-Length: 0

curl -I localhost:3000/no-content-type
HTTP/1.1 302 Found
Location: no content
Date: Wed, 28 Feb 2024 08:41:21 GMT
Content-Length: 0

What is the expected behavior?

I would like it to work as 'Content-Type', 'text/plain; charset=UTF-8'.

I am concerned if this should be addressed on the Adaptor side as it seems to be a problem on the Bun side.
oven-sh/bun#8530

What do you see instead?

No response

Additional information

I found this CI Bun from the failure.
#2247

In other words, this leads to the possibility that the specification "SSG to error on those without Content-type" may be difficult to implement due to the differences in Bun. There are three possible approaches

  1. fix Bun Adaptor.
  2. to fix SSG from hono/bun.
  3. temporarily forgo the creation of a Content-type check function for SSG.
@watany-dev watany-dev added the bug label Feb 28, 2024
@watany-dev
Copy link
Contributor Author

Note:

  1. is not appropriate as it will affect performance.

@usualoma
Copy link
Member

Sorry to bother you all - especially @watany-dev!
I was unaware that bun had such a specification.

  1. Even in SSG, if there is no Content-Type, it is assumed to be text/plain.

The "default is text/plain" seems a bit odd, but if hono itself has such a specification ( c.text() returns a response without Content-Type ), I think it makes sense to treat it as text/plan in SSG accordingly.

@yusukebe
Copy link
Member

yusukebe commented Mar 1, 2024

Hi @watany-dev @usualoma

As mentioned by @watany-dev, if we use c.text('foo') in a simple case, Hono does not add Content-Type:

app.get('/', (c) => c.text('foo'))

This is because the performance with and without Content-Type was significantly different when benchmarked in the past. Bun does not add Content-Type by default, but other runtimes add text/plain, so c.text() should be fine with the current specification.

When Hono didn't add Content-Type, it is expected to be text/plain, so I think @usualoma's "4" is a good choice.

@watany-dev
Copy link
Contributor Author

@yusukebe @usualoma
Thank you. I also agree with option No. 4, that is, the current specifications are fine. The use of StatusCode should be discussed in a separate issue. Here's what I think should be done regarding the related tasks going forward. We will adapt as better methods are proposed.

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

Successfully merging a pull request may close this issue.

3 participants