honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
20.34k stars 582 forks source link

`Content-Encoding` header is `gzip` without compression middleware #3449

Closed poeck closed 1 month ago

poeck commented 1 month ago

What version of Hono are you using?

4.5.8

What runtime/platform is your app running on?

Cloudflare Workers

What steps can reproduce the bug?

  1. Create a basic hono application without any middleware
  2. Add a route
  3. Call the route and check the headers

What is the expected behavior?

The Content-Encoding should probably not be present if there is no compress middle ware applied.

What do you see instead?

The Content-Encoding is automatically set to gzip.

Additional information

When trying to stream text with streamText from hono/streaming you need to manually add c.header("Content-Encoding", "none") for it to work correctly.

yusukebe commented 1 month ago

@poeck

It's not a bug. Cloudflare adds the content-encoding automatically. For instance, it reproduces with the code that does not use Hono:

export default {
  fetch: () => new Response('Hello'),
}
poeck commented 1 month ago

Oh, I see. Would it be worth considering overwriting this header for streams? I believe it's theoretically possible to stream data with gzip as well, but I couldn't get it to work quickly and it took me forever to figure out that this was the issue. 😅

To be honest, I don't know exactly how gzip works with HTTP, but is the header even correct, like is the response even gzipped? As soon as I overwrote the header with "none", the stream worked perfectly without me having to do any decoding myself (like gzip decoding, for example).

yusukebe commented 1 month ago

Hi @poeck

Thank you for the comment. Actually, it does not work on Cloudflare Workers without modifying the Content-Encoding header. In this case, it's good to set Identity instead of None:

c.header('Content-Encoding', 'Identity')

I can't find good documentation explaining Identity, but if it's set, it works well. It's used in the JSX Renderer.

I think it's a good idea to setContent-Encoding: 'Identity' in the streaming helper as default.

cc: @sor4chi @watany-dev What do you think of it?

sor4chi commented 1 month ago

Hi @yusukebe. I think it's a good idea. Unexpected behavior should be avoided as much as possible.

yusukebe commented 1 month ago

@sor4chi

I'm trying to add a change to set Content-Encoding: 'Identity' to streamText(). It's okay, but how about streamSSE()? I've tried using it on Cloudflare Workers without Content-Encoding: 'Identity', but it works well. Is it okay not to add the header? Do you have any idea?

yusukebe commented 1 month ago

In the streamSSE() case, it does not add a Content-Encoding header on Cloudflare Workers. So, I think it's okay that we don't add it to streamSSE(). Just streamText() is okay.

yusukebe commented 1 month ago

Mainly to @sor4chi

Thank you for your review of #3476.

I've re-thought about this issue.

As the issue on Cloudflare Workers SDK: https://github.com/cloudflare/workers-sdk/issues/6577, the fact that streaming is not enabled on Wrangler may be a Wrangler-side problem. This means streaming works well on Cloudflare's production, but it does not work on the current Wrangler.

If so, it's not good to fix this issue on the framework (Hono) side. The Cloudflare internal team has started investigating it, but we don't know when it will be resolved.

We can make some choices

  1. Implement #3476 as a workaround and remove this if the matter is fixed on Wrangler.
  2. Do not do anything now. But add the description "add the header Content-Encoding: 'Identity' on the Wrangler" on Streaming Helerp doc.

I prefer "2". What do you think of it?

sor4chi commented 1 month ago

Okay, I think 2 is fine with me too.

yusukebe commented 1 month ago

@poeck @sor4chi

I've added the description on the docs https://github.com/honojs/website/pull/507 to close this issue.

Thank you so much!