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

Allow using a custom compare function #1315

Closed
pLeminoq opened this issue Apr 4, 2023 · 7 comments · Fixed by #1431
Closed

Allow using a custom compare function #1315

pLeminoq opened this issue Apr 4, 2023 · 7 comments · Fixed by #1431
Labels
enhancement New feature or request PRs encouraged PRs are welcome if stakeholders want to take ownership
Milestone

Comments

@pLeminoq
Copy link

pLeminoq commented Apr 4, 2023

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.

@pLeminoq pLeminoq added the enhancement New feature or request label Apr 4, 2023
@vincenzopalazzo vincenzopalazzo added this to the v5.2.0 milestone Apr 11, 2023
@vincenzopalazzo vincenzopalazzo self-assigned this Apr 11, 2023
@DivineThreepwood
Copy link

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

@vincenzopalazzo
Copy link
Collaborator

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!

@vincenzopalazzo vincenzopalazzo added the PRs encouraged PRs are welcome if stakeholders want to take ownership label Jul 20, 2023
@vincenzopalazzo vincenzopalazzo removed their assignment Jul 20, 2023
@RobertBrunhage
Copy link

RobertBrunhage commented Feb 26, 2024

@vincenzopalazzo not the most accurate but I made a fork RobertBrunhage@0e797be 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
Copy link
Contributor

kvenn commented May 15, 2024

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
Copy link
Contributor

kvenn commented May 15, 2024

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

@kvenn
Copy link
Contributor

kvenn commented May 15, 2024

Draft PR: #1431

@RobertBrunhage
Copy link

@kvenn Not seen an issue as of yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PRs encouraged PRs are welcome if stakeholders want to take ownership
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants