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

XHR: set lengthComputable event property to false if computing length failed #617

Open
kettanaito opened this issue Aug 13, 2024 · 2 comments
Labels
good first issue Good for newcomers

Comments

@kettanaito
Copy link
Member

With #613 and #616, we've improved how the XHR interceptor calculates the total length of the request/response bodies. There are, however, cases when that length won't be calculated. For example, if getBodyByteLength() function fails (e.g. in Node.js v18.19 structuredClone() throws on cloning Undici Response instances; this has been fixed in later versions).

Per specification, there are also cases when calculating the total length is impossible. I suspect that's a streaming scenario, when the server itself doesn't know the total length of the streamed response body.

In either of those cases, we should set the lengthComputable property on request/response events to false. If the length is computable (default behavior right now), the property must be set to true (we may not be doing that right now).

Materials

@kettanaito kettanaito added the good first issue Good for newcomers label Aug 13, 2024
@cu8code
Copy link

cu8code commented Oct 11, 2024

Hey, is this issue available! Would love to help!

@cu8code
Copy link

cu8code commented Oct 11, 2024

@kettanaito I am planning to add two checks
at

const totalRequestBodyLength = await getBodyByteLength(

like this

const totalRequestBodyLength = await getBodyByteLength(this[kFetchRequest]);
    const isRequestLengthComputable = totalRequestBodyLength !== null;

    this.trigger('loadstart', this.request.upload, {
        loaded: 0,
        total: isRequestLengthComputable ? totalRequestBodyLength : 0,
        lengthComputable: isRequestLengthComputable,
    });

and

const totalResponseBodyLength = await getBodyByteLength(response.clone())

const totalResponseBodyLength = await getBodyByteLength(response.clone());
    const isResponseLengthComputable = totalResponseBodyLength !== null;

    this.trigger('loadstart', this.request, {
        loaded: 0,
        total: isResponseLengthComputable ? totalResponseBodyLength : 0,
        lengthComputable: isResponseLengthComputable,
    });

Would like if you can review my approach!

Also, I wanted to add some test case, but I am a bit confused about how to set up a test for this! There are different runtimes, do I need to write test for individual runtimes! Ex node-browser, also how to structure it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants