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

fix: Remove content-encoding header from already decompressed responses #54

Merged
merged 2 commits into from
Feb 14, 2024

Conversation

benbrandt
Copy link

When the compress flag is set to true, the fetch implementation sends an Accept-Encoding header and decompresses the response.

However, the decompressed body is put into a Response that still has a Content-Encoding that no longer matches the actual encoding of the body.

This causes issues if this Response object is used downstream (i.e. sent back to a browser), and there is another attempt to decode the body based on the content-encoding header.

When the `compress` flag is set to true, the fetch implementation sends an Accept-Encoding header and decompresses the response.

However, the decompressed body is put into a `Response` that still has a `Content-Encoding` that no longer matches the actual encoding of the body.

This causes issues if this `Response` object is used downstream (i.e. sent back to a browser), and there is another attempt to decode the body based on the content-encoding header.
@changeset-bot
Copy link

changeset-bot bot commented Sep 28, 2023

🦋 Changeset detected

Latest commit: 894adcb

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@remix-run/web-fetch Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benbrandt
Copy link
Author

@brophdawg11 does this change make sense? Or is there more I need to do around the changeset to get this ready for review?

@brophdawg11 brophdawg11 self-requested a review November 16, 2023 13:31
@jacob-ebey
Copy link
Member

@benbrandt This behavior is unspecified by the spec as far as I can tell, and there is open discussion happening here around the topic here: wintercg/fetch#23

This makes sense to me though and I'm happy to get this merged to generally ease the use of fetch in a server environment.

@benbrandt
Copy link
Author

@jacob-ebey thanks for the link. Definitely not a straightforward topic by any means. Happy to see it merged if it makes sense on your end. Thanks for the help!

@jacob-ebey jacob-ebey merged commit 822a3c3 into remix-run:main Feb 14, 2024
65 checks passed
@jacob-ebey
Copy link
Member

I'm going to do some spec reading and exploration, but I'm having second thoughts on this.... We (remix's fetch polyfill) used to recommend decoding and passing on the relevant data in the docs at some point, or returning a raw object from fetch().then(r => r.json()) to get around this. The previous recommendations are spec compliant and this is potentially not if a decision is made to define this behavior. The root of this concern isn't directly related to what this enables, but rather an underlying "is fetch() capable of xyz" where the xyz is "consume a raw compressed body for further processing".

I want others to weigh in here.

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

Successfully merging this pull request may close these issues.

2 participants