Improve body size estimation #734
Conversation
test/body-size.ts
Outdated
|
|
||
| async function testBodySize(t: ExecutionContext, body: unknown) { | ||
| const actualSize = getBodySize(body); | ||
| const expectedText = await new Response(body).text(); |
There was a problem hiding this comment.
These tests are mostly just mimicking the current implementation to test the internal utilities. I would prefer that they use a server and Ky itself, as most of the other tests do, to echo back the de-serialized body length that it receives and maybe also the Content-Length, for example, and then run assertions for different body types based on that and the totalBytes parameter. Thus making the assertions more realistic and less coupled to the implementation.
| const encoder = new TextEncoder(); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/ban-types | ||
| export const getBodySize = (body?: BodyInit | null): number => { |
There was a problem hiding this comment.
Could we simplify this whole function by using a temporary response, like you did for forms?
const buffer = await new Response(body).arrayBuffer();
return buffer.byteLength;There was a problem hiding this comment.
Using Response to get the size consumes the entire body, as opposed to using size/length properties which we currently do when possible.
We don't want ky to consume the entire request body twice, as opposed to the underlying fetch API that doesn't do that. If we had to, and this might not be so bad, is we might as well pass our constructed buffer right on through to the fetch request.
There was a problem hiding this comment.
You're right that we don't want to consume the stream if the body is a stream. But we can't get totalBytes for a stream anyway. So I was thinking if the body is a stream, short-circuit with a size of 0.
There was a problem hiding this comment.
Yes, that's what we already do - ReadableStreams pass right through and return 0.
What I'm referring to is ArrayBuffers, or Files/Blobs in FormData, which contain a size property, and are also iterable. The latter is done by new Response, the former is what we do.
There was a problem hiding this comment.
I know that we already skip size checks for streams. And you have maintained that with these changes, which is great. But that's why your reply about consuming the body is confusing to me. All non-stream body types are simple objects that are not permanently consumed the way streams are.
If you mean that you want to avoid extra iterations on the body, as in O(n) vs O(n*2) performance, I think that's reasonable. But our main problem at the moment is correctness, not performance, and the network is going to be the limiting factor for performance. IMO, if we can leverage Response to calculate the size, then we should.
There are also performance optimizations we could make, like only using Response to calculate the size for complex types like FormData. We could also re-use the ArrayBuffer that it generates in the process.
|
@Richienb any further thoughts on those comments about the tests and/or using |
|
Not sure what to do from here. |
|
Can we move this forward? I suggest keep the current PR implementation (manual calculation) with two improvements:
|
| size += encoder.encode(`; filename="${value.name ?? 'blob'}"`).byteLength; | ||
| size += encoder.encode(`\r\nContent-Type: ${value.type || 'application/octet-stream'}`).byteLength; |
There was a problem hiding this comment.
Another improvement I want to make is use difference calculations instead of hard-coding these strings
3 participants
Note that I needed to adjust some tests because they expect undershooting the size, but this is the first time the size is actually exactly right.
Possibly related to #694