-
Notifications
You must be signed in to change notification settings - Fork 537
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
feat: add decompress interceptor #3274
base: main
Are you sure you want to change the base?
feat: add decompress interceptor #3274
Conversation
938fa4b
to
df4804c
Compare
Should we also handle rawdeflate? |
Yep, I'm using the existing |
lib/interceptor/decompress.js
Outdated
this.#inputStream, | ||
...restDecoders, | ||
this.#onDecompressStreamFinished.bind(this) | ||
).on('data', (chunk) => this.#handler.onData(chunk)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing backpressure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation seems lost
} | ||
} | ||
|
||
function createDecompressionInterceptor () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might need to support add the hinting of supported encoding in case implementers wants to limit this (I can only think for performance reasons).
They can pass a set of encodings supported, and the decompress decorator hints it using accept-encoding
outputStream.on('data', (chunk) => { | ||
if (!this.#handler.onData(chunk)) { | ||
this.#inputStream.pause() | ||
this.#inputStream.once('drain', () => this.#inputStream.resume()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those is wrong. Drain is signaled through the resume function sent to onHeaders
// https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Encoding | ||
if (requestEncodings.length !== 0 && method !== 'HEAD' && method !== 'CONNECT' && !nullBodyStatus.includes(statusCode)) { | ||
const decoders = [] | ||
for (let i = 0; i < requestEncodings.length; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for (let i = 0; i < requestEncodings.length; ++i) { | |
for (let i = requestEncodings.length - 1; i >= 0; --i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way round, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I made the same changes #3343.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'll come back to this PR this week
Added a new interceptor to decompress the response body. I lifted some of the implementation from the way decompression is handled in the fetch client
This relates to...
Rationale
Changes
Features
Bug Fixes
Breaking Changes and Deprecations
Status