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

Expose "Content-Encoding" handling publicly #3412

Open
kettanaito opened this issue Jul 21, 2024 · 13 comments · May be fixed by #3423
Open

Expose "Content-Encoding" handling publicly #3412

kettanaito opened this issue Jul 21, 2024 · 13 comments · May be fixed by #3423
Labels
enhancement New feature or request

Comments

@kettanaito
Copy link
Contributor

kettanaito commented Jul 21, 2024

This would solve...

Hi! In Interceptors, we are looking at improving the compatibility of response handling for mocked responses. As a part of that effort, we'd like to produce an identical behavior that Undici exhibits with compressed response bodies.

The implementation should look like...

If the following handling logic is a tad abstracted and exposed as a standalone function, it could help us a lot to stay consistent and make countless developers' tests more reliable:

undici/lib/web/fetch/index.js

Lines 2139 to 2161 in c1f7d2a

if (codings.length !== 0 && request.method !== 'HEAD' && request.method !== 'CONNECT' && !nullBodyStatus.includes(status) && !willFollow) {
for (let i = codings.length - 1; i >= 0; --i) {
const coding = codings[i]
// https://www.rfc-editor.org/rfc/rfc9112.html#section-7.2
if (coding === 'x-gzip' || coding === 'gzip') {
decoders.push(zlib.createGunzip({
// Be less strict when decoding compressed responses, since sometimes
// servers send slightly invalid responses that are still accepted
// by common browsers.
// Always using Z_SYNC_FLUSH is what cURL does.
flush: zlib.constants.Z_SYNC_FLUSH,
finishFlush: zlib.constants.Z_SYNC_FLUSH
}))
} else if (coding === 'deflate') {
decoders.push(createInflate())
} else if (coding === 'br') {
decoders.push(zlib.createBrotliDecompress())
} else {
decoders.length = 0
break
}
}
}

This is a brief look at the implementation. It may require more effort than just abstracting this code block. We can discuss the exact API in detail together. I imagine it being something like decode(response: Response): Decoder[]. Perhaps exposing the pipeline would make for an even easier consumption.

I have also considered...

We are considering writing the Content-Encoding handling by hand but I'd like to join efforts on this one. I don't wish to repeat things. I like using the platform. I like Undici.

Additional context

I'm interested in opening a pull request and seeing this through, given it's something you agree on. Let me know.

@mcollina
Copy link
Member

@KhafraDev wdyt?

@KhafraDev
Copy link
Member

Sounds fine to me.

@kettanaito
Copy link
Contributor Author

kettanaito commented Jul 21, 2024

Thank you for your timely responses, @mcollina, @KhafraDev!

I think a utility function like this would make the most sense:

export function decompress(response: Response) {
  const decoders = []
  // ...populate the `codings` array based on the passed response.

  // Expose the entire pipeline since `pipeline` itself is likely
  // too low-level to export from Undici. These would also be called
  // together anyway. 
  return pipeline(response.body, ...decoders, () => {})
}

I'd love to hear your thoughts on whether response instance is enough to decide that. I can see request references in that code snippet above as well.

@KhafraDev
Copy link
Member

KhafraDev commented Jul 21, 2024

#3274 might solve this, I'd need to see some uses for a standalone decompress.

@kettanaito
Copy link
Contributor Author

On the consumption side of things, it would be nice if this behavior was imported from Node.js to be consistent with whichever Undici version is used in a particular Node.js version. In other words, so it would always align with the Node.js version used by the consumer of Interceptors.

Granted, the decompression logic isn't supposed to change often. I'm mostly thinking bug fixes here.

@kettanaito
Copy link
Contributor Author

kettanaito commented Jul 21, 2024

@KhafraDev, would that allow us to use decompression without any sort of Undici interceptor being created? That'd be ideal. The functionality itself doesn't need an interceptor, it can be a utility function that deals with the body transformations based on the Content-Encoding header. Compressed body in, properly decompressed body out. Then, the same utility can be reused in the interceptor pull request you've mentioned, I'd hope.

@PandaWorker
Copy link

PandaWorker commented Jul 21, 2024

I think we need to split compression and decompression into separate interceptors and add decompression parameters to request options the decompress: boolean, compress: boolean. Also enable them by default

import { request } from 'undici';

async function test(){
  const resp = await request('https://example.site/', {
    method: 'POST',
    body: `qwerty123`,
    
    decompress: true,
    compress: true
  });
  
  // decompressed response data
  return await resp.body.text();
}

OR

import { request, Agent, interceptors } from 'undici';

const dispatcher = new Agent().compose(interceptors.decompress()).compose(interceptors.compress(['gzip', 'br']));

async function test(){
  const resp = await request('https://example.site/', {
    method: 'POST',
    body: `qwerty123`,
    
    decompress: true,
    compress: true,
    dispatcher: dispatcher
  });
  
  // decompressed response data
  return await resp.body.text();
}

@kettanaito
Copy link
Contributor Author

@PandaWorker, as long as the decompression is exposed as a standalone utility. This is what this proposal is about. The way we mean to use it in Interceptors (which are not Undici interceptors, pardon for the naming confusion) is with arbitrary response bodies. What I'm proposing here is likely more low-level than what you have in mind, so two may combine, in the end.

@metcoder95
Copy link
Member

If the goal is to expose the decompression as utility #3274 won't be what might close this issue, as it attempts to expose it through an interceptor to be used with dispatcher.compose.

They are slightly interconnected but not necessarily the same (specially because this issue aims to have an utility that works with WHATWG Response object I assume). So a PR aiming just for that will be the best

@kettanaito
Copy link
Contributor Author

@metcoder95, agreed. #3274 can reuse the utility function I am bringing up as a part of this suggestion. I'm glad to hear we are aligned on that!

If there are no objections, I can give this a go when I have a minute (likely next week). A code review and a bit of guidance will be appreciated.

@kettanaito kettanaito linked a pull request Jul 26, 2024 that will close this issue
15 tasks
@kettanaito
Copy link
Contributor Author

Got excited by this, started with it sooner. The pull request is at #3423, extremely early.

@mikicho
Copy link

mikicho commented Sep 30, 2024

Hey there :)
Can I help merge this in?

@kettanaito
Copy link
Contributor Author

Hey, @mikicho 👋 Yes! I'm short on time these weeks, but I'm keeping a list of remaining todos on the pull request: #3423.

I could use some help with type-testing the newly added utilities and looking into this odd issue that somehow unrelated tests are failing now just because I moved the utilities under a different directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants