Skip to content

feat(graphql): add asyncDeepEquals to the graphqlClient #1496

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Neelansh-ns
Copy link

@Neelansh-ns Neelansh-ns commented May 29, 2025

Fixes / Enhancements

  • Added asyncDeepEquals param to the GraphqlClient as the DeepCollectionEquality is an expensive operation and was running synchronously. This in turn would result in a jank whenever there is a response received and the equality check happens in the _cachedDataHasChangedFor function.
  • To avoid the jank the developer can pass the async function for equality check, like compute() to move the operation to a different isolate.
  • This also partially fixes Jank on every query #1196
Before After

@Neelansh-ns Neelansh-ns force-pushed the feat/async-deep-equals branch from ebb3b2f to 5188dd7 Compare May 29, 2025 10:16
@vincenzopalazzo
Copy link
Collaborator

While Ci is running, can you please adjust the commit message according with https://github.com/zino-hofmann/graphql-flutter/blob/main/docs/dev/MAINTAINERS.md

Thanks!

@Neelansh-ns Neelansh-ns changed the title add asyncDeepEquals to the graphqlClient feat(graphql): add asyncDeepEquals to the graphqlClient May 29, 2025
@Neelansh-ns Neelansh-ns force-pushed the feat/async-deep-equals branch from 5188dd7 to 821aa3b Compare May 29, 2025 11:30
@Neelansh-ns
Copy link
Author

While Ci is running, can you please adjust the commit message according with https://github.com/zino-hofmann/graphql-flutter/blob/main/docs/dev/MAINTAINERS.md

Thanks!

Done, please review.

@vincenzopalazzo vincenzopalazzo requested a review from Copilot May 30, 2025 08:33
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces an asynchronous deep equality check to improve performance by offloading expensive synchronous operations to another isolate. The key changes include:

  • Adding the asyncDeepEquals parameter to GraphQLClient and QueryManager.
  • Updating rebroadcast methods and their invocations from synchronous to their asynchronous counterparts.
  • Adjusting documentation and function names to reflect the async changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
packages/graphql/lib/src/graphql_client.dart Added asyncDeepEquals parameter and updated rebroadcast calls.
packages/graphql/lib/src/core/query_manager.dart Introduced AsyncDeepEqualsFn and revised rebroadcast logic.
packages/graphql/lib/src/core/observable_query.dart Updated calls to the async rebroadcast method.

@vincenzopalazzo
Copy link
Collaborator

@Neelansh-ns before jumping in the review, any comment on the copilot review?

Thanks

@Neelansh-ns
Copy link
Author

@Neelansh-ns before jumping in the review, any comment on the copilot review?

Thanks

Accepted one suggestion, the other can be ignored. Added the reasoning in the reply to the suggestion.

@vincenzopalazzo
Copy link
Collaborator

Cool, now the commit needs to be squashed by preserving the Co-authored-by: Copilot <[email protected]> inside the commit body

fix: add .ignore() to fire and forget

Co-authored-by: Copilot <[email protected]>
@Neelansh-ns Neelansh-ns force-pushed the feat/async-deep-equals branch from 7bb0d93 to 5d0b17b Compare May 30, 2025 18:46
@Neelansh-ns
Copy link
Author

Cool, now the commit needs to be squashed by preserving the Co-authored-by: Copilot <[email protected]> inside the commit body

Done!

@Neelansh-ns
Copy link
Author

Hi @vincenzopalazzo, kindly review and let me know your thoughts. If all looks good, can we aim for pushing it to pub?

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.

Jank on every query
3 participants