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

🐛 BUG: Brotli response not decoded #1587

Open
hansottowirtz opened this issue Jan 28, 2024 · 3 comments
Open

🐛 BUG: Brotli response not decoded #1587

hansottowirtz opened this issue Jan 28, 2024 · 3 comments
Labels
feature request Request for Workers team to add a feature

Comments

@hansottowirtz
Copy link

Which Cloudflare product(s) does this pertain to?

Miniflare

What version(s) of the tool(s) are you using?

^3.20231218.4

What version of Node are you using?

18.19.0

What operating system are you using?

Mac

Describe the Bug

The result of res.text() of a response with content-encoding: br is not decoded, but when it is gzip, it is correctly decoded.

This worker returns a mess:

export default {
  async fetch() {
    const res = await fetch("https://blog.cloudflare.com/this-is-brotli-from-origin", {
      headers: {
        "accept-encoding": "br, gzip, deflate"
      }
    });
    return new Response(await res.text(), {
      headers: {
        "content-type": "text/html"
      }
    });
  }
}

This worker returns the correct HTML:

export default {
  async fetch() {
    const res = await fetch("https://blog.cloudflare.com/this-is-brotli-from-origin", {
      headers: {
        "accept-encoding": "gzip, deflate"
      }
    });
    return new Response(await res.text(), {
      headers: {
        "content-type": "text/html"
      }
    });
  }
}

Please provide a link to a minimal reproduction

No response

Please provide any relevant error logs

No response

@hansottowirtz hansottowirtz added the bug Something isn't working label Jan 28, 2024
@penalosa penalosa transferred this issue from cloudflare/workers-sdk Jan 29, 2024
@kentonv
Copy link
Member

kentonv commented Jan 31, 2024

This is more of a feature request than a bug. The spec does not require brotli support.

That said, we actually do have this support, but it is still experimental. You can enable brotli support by setting the brotli_content_encoding compatibilty flag. Note this flag is not yet documented nor enabled by default as I think there are some obscure bad interactions with other parts of Cloudfare's stack that we were waiting on fixes to. But you can try it, and if it works in your use case, it works.

@fhanau may know more about what the remaining issues are.

@fhanau
Copy link
Collaborator

fhanau commented Jan 31, 2024

This is more of a feature request than a bug. The spec does not require brotli support.

That said, we actually do have this support, but it is still experimental. You can enable brotli support by setting the brotli_content_encoding compatibilty flag. Note this flag is not yet documented nor enabled by default as I think there are some obscure bad interactions with other parts of Cloudfare's stack that we were waiting on fixes to. But you can try it, and if it works in your use case, it works.

@fhanau may know more about what the remaining issues are.

Yes, that's a good description of the issue. As Kenton noted, you may experience different behavior in the production runtime than in local testing due to incomplete brotli support Cloudfare's stack. Notably, you will not be able to fetch using the brotli content encoding in production even when you add it to accept-encoding at this time, so currently the flag is still experimental and primarily useful for local testing.

@jasnell
Copy link
Member

jasnell commented Feb 2, 2024

@fhanau ... I wonder if it would be worthwhile adding a warning when br is specified without the flag / or in production. I know that warnings often go unnoticed so it's of limited value, but while we're waiting for the production stack issues to be resolved it's something.

@jasnell jasnell added feature request Request for Workers team to add a feature and removed bug Something isn't working labels Feb 2, 2024
@penalosa penalosa removed this from workers-sdk Jul 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Request for Workers team to add a feature
Projects
None yet
Development

No branches or pull requests

4 participants