-
Notifications
You must be signed in to change notification settings - Fork 355
Network req data export improvements #9423
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
base: master
Are you sure you want to change the base?
Network req data export improvements #9423
Conversation
_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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this encode
and not decode
?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly to comment above, create helper function to use here and above.
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.
Added a helper function, will be adding the tests soon.
notifyListeners(); | ||
} | ||
} finally { | ||
isFetchingFullData = false; | ||
} | ||
} | ||
|
||
//TODO check if all cases are handled correctly |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should belong in network/utils/http_utils.dart
. Please also add tests. Thank you!
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.
moved the helper function, will be adding the tests soon.
Please add a note to
packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
if 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.