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: fetch support compressed responses #2591

Merged
merged 9 commits into from Feb 24, 2024
Merged

Conversation

mikicho
Copy link
Contributor

@mikicho mikicho commented Feb 24, 2024

fixes #2397 (comment)

This is a pretty naive solution that doesn't support more decoders easily (or multiple decoders at all), but I'm not expecting it in streams and such.
Improvements are welcome.

@mikicho mikicho requested a review from gr2m February 24, 2024 15:27
Copy link
Member

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about deflate and co?

@mikicho
Copy link
Contributor Author

mikicho commented Feb 24, 2024

@Uzlopak I can add support for deflate and br(?), but in this ugly(?) if/else style, it would be complex to support all combinations (it's possible to have more than one compression)
Do you have any ideas on how we can use streams? I tried to do something like:

const decoder = []
const codings = message.headers['content-encoding'].split(',')
for (const coding in codings) {
  if (coding === 'gzip') decoder.push(zlib.createGunzip())
  if (coding === 'deflate') decoder.push(..)
}

new ReadableStream({
  start(controller) {
    message.on('data', chunk => pipeline(Readable.from(chunk), ...decoders. () => {})
  }
})

But I got an error. I'm not an expert in streams, and I'm clearly missing something.

@Uzlopak
Copy link
Member

Uzlopak commented Feb 24, 2024

Maybe here you can get some clues how we do it in undici.

nodejs/undici#2650

@mikicho
Copy link
Contributor Author

mikicho commented Feb 24, 2024

@Uzlopak Thanks! I succeed to make it work.
I piled up the chunks because this is cleaner(?) then create promises and await for them to complete.

lib/create_response.js Outdated Show resolved Hide resolved
@Uzlopak Uzlopak changed the title fix: fetch support gzip response fix: fetch support compressed responses Feb 24, 2024
Copy link
Member

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BEAUTIFUL

@mikicho mikicho merged commit 991a8f3 into beta Feb 24, 2024
14 checks passed
@mikicho mikicho deleted the Michael/fetch-support-gzip branch February 24, 2024 22:51
Copy link

🎉 This PR is included in version 14.0.0-beta.4 🎉

The release is available on:

Your semantic-release bot 📦🚀

@Uzlopak
Copy link
Member

Uzlopak commented Feb 24, 2024

teamwork makes the dream work

@mikicho
Copy link
Contributor Author

mikicho commented Feb 24, 2024

image

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

Successfully merging this pull request may close these issues.

None yet

2 participants