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

Offline support for Network page #8332

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

hrajwade96
Copy link
Contributor

@hrajwade96 hrajwade96 commented Sep 21, 2024

Adding offline support to Network page.

issue link - #4470

List which issues are fixed by this PR.

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

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read the Flutter Style Guide recently, and have followed its advice.
  • I signed the CLA.
  • I listed at least one issue that this PR fixes in the description above.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or there is a reason for not adding tests.

build.yaml badge

If you need help, consider asking for help on Discord.

@hrajwade96 hrajwade96 marked this pull request as draft September 21, 2024 09:19
@hrajwade96 hrajwade96 changed the title added initial implementation Offline support for Network page Sep 21, 2024
@kenzieschmoll
Copy link
Member

@hrajwade96 is this PR ready for review or is this still a work in progress?

@hrajwade96
Copy link
Contributor Author

@hrajwade96 is this PR ready for review or is this still a work in progress?

@kenzieschmoll just finishing up few things, I will mark it ready soon

@hrajwade96 hrajwade96 marked this pull request as ready for review October 2, 2024 17:19
shouldLoad: (data) => !data.isEmpty,
loadData: (data) => loadOfflineData(data),
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code after this if statement should be put in an else block

Copy link
Contributor Author

@hrajwade96 hrajwade96 Oct 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not using else was an oversight, added else block now, and also now there is no code outside.

Comment on lines 26 to 27
List<Socket>? socketReqData = [];
socketReqData = json[OfflineDataKeys.socketData.name] as List<Socket>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

define in one step:
final socketRequestData =(json[OfflineDataKeys.socketData.name] as List?)?.cast<Socket>();

Also, prefer socketRequestData over socketReqData to avoid abbreviations in variable names

Comment on lines 24 to 25
final httpRequestData = json[OfflineDataKeys.httpRequestData.name]
as List<DartIOHttpRequestData>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cast this to the proper type instead:
final httpRequestData = (json[OfflineDataKeys.httpRequestData.name] as List?)?.cast<DartIOHttpRequestData>();

Copy link
Contributor Author

@hrajwade96 hrajwade96 Oct 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

now I have explicitly called fromJson on each object from list, will refactor it and make it into an extension.
BTW one observation is when I remove/comment all the toJson() of all the required classes, and pass the list of DartIOHttpRequestData directly

//To Json
  Map<String, dynamic> toJson() {
    return {
      OfflineDataKeys.httpRequestData.name: httpRequestData,
      OfflineDataKeys.selectedRequestId.name: selectedRequestId,
      OfflineDataKeys.socketData.name: socketData,
    };
// From Json    
     final httpRequestData = json[OfflineDataKeys.httpRequestData.name]
    as List<DartIOHttpRequestData>;
return OfflineNetworkData(
      httpRequestData: httpRequestData,
      selectedRequestId:
      json[OfflineDataKeys.selectedRequestId.name] as String?,
      socketData: socketReqData,
    )

This still works, as dart is preserving those object types (when we don't serialise them, hence no need for deserialisation).
Since this offline feature is only used in dart (not passed to any external system) and an in-memory thing, is this a good idea to not fully serialise & de-serialise it? and store the inner objects of DartIOHttpRequestData list and Socket and as they are and pick them? Or is it not recommended or safe to use?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In order to download the screen's data as a JSON file, the serialization is required. However, not serializing the data is acceptable for the 'review history' feature, which allows continuing to view recent data offline when the connected app disconnects. I wouldn't worry about distinguishing the two for this PR. This can be a clean up later if the serialization / deserialization for the 'review history' features causes performance issues.

@hrajwade96 hrajwade96 marked this pull request as draft October 23, 2024 15:13
@hrajwade96 hrajwade96 marked this pull request as ready for review October 27, 2024 14:32
SocketJsonKey.id.name: id,
SocketJsonKey.startTime.name: startTime,
SocketJsonKey.endTime.name: endTime,
//TODO verify if these timings are in correct format
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address this TODO.

Copy link
Member

@kenzieschmoll kenzieschmoll left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add test coverage for this change. Thanks!

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

Successfully merging this pull request may close these issues.

2 participants