Network req data export improvements#9423
Conversation
hrajwade96
requested review from
a team,
bkonyi and
kenzieschmoll
as code owners
| _responseBody = utf8.decode(fullRequest.responseBody!); | ||
| _requestBody = utf8.decode(fullRequest.requestBody!); | ||
| var responseMime = | ||
| responseHeaders?['content-type']?.toString().split(';').first; |
There was a problem hiding this comment.
Could we pull this into a helper function (e.g. getHeadersMimeType)? That way we can use it both here and below. Please add it to network/utils/http_utils.dart and add tests. Thanks!
There was a problem hiding this comment.
Added a helper function, will be adding the tests soon.
| if (isTextMimeType(responseMime)) { | ||
| _responseBody = utf8.decode(fullRequest.responseBody!); | ||
| } else { | ||
| _responseBody = base64.encode(fullRequest.responseBody!); |
There was a problem hiding this comment.
Why is this encode and not decode?
There was a problem hiding this comment.
For text-based data like json, HTML, or XML, I've used utf8.decode since it can be directly represented and rendered without additional serialization.
For binary data such as Pdf, Pngs, or other non-text formats, I've used Base64 encode on the raw bytes so they can be safely included in the JSON-based HAR export.
When importing the HAR into a tool e.g. Chrome DevTools, the tool will automatically decode this Base64 back into its original binary form (guided by the "encoding": "base64" field present in the HAR).
| } | ||
|
|
||
| if (fullRequest.requestBody != null) { | ||
| if (isTextMimeType(requestMime)) { |
There was a problem hiding this comment.
Similarly to comment above, create helper function to use here and above.
There was a problem hiding this comment.
Added a helper function, will be adding the tests soon.
| } | ||
| } | ||
|
|
||
| //TODO check if all cases are handled correctly |
There was a problem hiding this comment.
Please address the TODO, thanks!
| /// This function is useful for determining whether the content of an HTTP | ||
| /// request or response can be directly included in a HAR or JSON file as | ||
| /// human-readable text. | ||
| bool isTextMimeType(String? mimeType) { |
There was a problem hiding this comment.
I think this should belong in network/utils/http_utils.dart. Please also add tests. Thank you!
There was a problem hiding this comment.
moved the helper function, will be adding the tests soon.
|
Hello @hrajwade96 - is this PR still on your radar or should it be closed? Thank you! |
Hi @elliette, yes, I’m planning to finish this soon and will start making the remaining changes. Apologies for the gap in between! |
@elliette I’ve addressed all the comments, could you please take another look at the PR? |
|
Greetings from stale PR triage! 👋 |
3 participants
Please add a note to
packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.mdif your change requires release notes. Otherwise, add the 'release-notes-not-required' label to the PR.Pre-launch Checklist
///).If you need help, consider asking for help on Discord.