zino-hofmann / graphql-flutter

A GraphQL client for Flutter, bringing all the features from a modern GraphQL client to one easy to use package.
https://zino-hofmann.github.io/graphql-flutter
MIT License
3.24k stars 612 forks source link

Allow using a custom compare function #1315

Closed pLeminoq closed 1 month ago

pLeminoq commented 1 year ago

Is your feature request related to a problem? Please describe. As pointed out in this comment using DeepCollectionEquality().equals to check if cached data has changed is slow.

Describe the solution you'd like I want Query-/Mutation-/SubscriptionOptions to have a compare function parameter which I can provide. I think a common pattern could be to use the parseFn to build a compare function which parses the results before comparing.

Additional context I already implemented this feature for testing purposes on a fork and in our case it considerably improves performance. I would like to get feedback on the idea before opening a PR.

DivineThreepwood commented 1 year ago

@vincenzopalazzo could you please give @pLeminoq feedback how we should proceed here?

vincenzopalazzo commented 1 year ago

I want to take a look at the PR; there is no feedback to give here, right? If the compare function is slow, we provide a custom one, but we also need to prove that this fixes the problem.

If the PR convinces me that it solves the problem (maybe with some data report), this would be good to add, but I can not say anything more. I see no code yet!

RobertBrunhage commented 6 months ago

@vincenzopalazzo not the most accurate but I made a fork https://github.com/RobertBrunhage/graphql-flutter/commit/0e797bef6c5e5cb7b3ee04938015c3b664b30c21 where I did some manual pagination tests.

Saw a 10x improvement in speed and all the "jank" was gone when paginating.

no change (13% of total time) above PR change (1.5% of total time spent)
Screenshot 2024-02-24 at 16 10 34 Screenshot 2024-02-26 at 08 03 10

Note: the testing wasn't that accurate, I mainly ran both in profile mode and scrolled on a page we had a jank in for every pagination request. The jank on my devices wasn't noticeable anymore.

It would be nice to pass a "parsefn" but improving the built-in would be highly valuable IMO

kvenn commented 3 months ago

This improved performance from 150ms for a writeFragment -> 21ms (still dropped frames, but MUCH better). What if (just as a first pass) you allow passing in an deep equals to the graphql library. It's already defined as a global!

Then we could provide @RobertBrunhage's function as a stop gap. Eventually, this could be a Future so we can move it to an isolate on our own. But that might be a bit more work...

@vincenzopalazzo if you're okay with this, I can open a PR.

typedef DeepEqualsFn = bool Function(dynamic a, dynamic b)
DeepEqualsFn _deepEquals =
    const DeepCollectionEquality().equals;

class QueryManager {
    return GraphQLClient(
      cache: _cache,
      link: link,
      equalityCheck: customDeepEquals
    );
  GraphQLClient({
    required this.link,
    required this.cache,
    DefaultPolicies? defaultPolicies,
    bool alwaysRebroadcast = false,
    DeepEqualsFn? deepEquals,
  })  : defaultPolicies = defaultPolicies ?? DefaultPolicies(),
        queryManager = QueryManager(
          link: link,
          cache: cache,
          alwaysRebroadcast: alwaysRebroadcast,
        ){
    _deepEquals = deepEquals ?? _deepEquals;
}
kvenn commented 3 months ago

Also @RobertBrunhage, have you run into any issues with the custom one you provided?

kvenn commented 3 months ago

Draft PR: https://github.com/zino-hofmann/graphql-flutter/pull/1431

RobertBrunhage commented 3 months ago

@kvenn Not seen an issue as of yet