-
Notifications
You must be signed in to change notification settings - Fork 143
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: body capture handling #1026
Conversation
pauldambra
commented
Feb 15, 2024
•
edited
Loading
edited
- reads request body more safely for fetch
- improves XHR request and response body reading
- removes the need for content-length header for reading payloads
Size Change: +2.17 kB (0%) Total Size: 862 kB
ℹ️ View Unchanged
|
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 have multiple almost identical linters now, but i'll deal with that in a separate PR
let requestContentLength: string | number = headers?.['content-length'] || estimateBytes(payload) | ||
if (_isString(requestContentLength)) { |
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.
turns out we rarely/never have the content-length header for requests
so we have to have a way to estimate body size when there is no header
changes elsewhere in this PR meant we know this is always a string now so that's much simpler than I was worrying
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.
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.
Ah, not that the request won't have it but fetch
doesn't expose computed request headers to code here. So, we can't read content-length regardless of whether the browser adds it when sending.
I tried not reading the headers until after the request had been made and couldn't read request headers that were visible in the network panel of the browser
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.
I see, gotcha.
src/loader-recorder-v2.ts
Outdated
* According to MDN https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/response | ||
* @param response an ArrayBuffer, a Blob, a Document, a JavaScript object, or a string, depending on the value of XMLHttpRequest.responseType, that contains the response entity body. | ||
*/ | ||
function _tryReadXHRResponseBody(response: any): string | null { |
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.
in each of these _tryReadX
methods we can ignore body types we don't want to process e.g. ArrayBuffer