feat(graphql): Support custom equality function for cache comparison

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.25k stars 622 forks source link

feat(graphql): Support custom equality function for cache comparison #1431

Closed kvenn closed 3 months ago

kvenn commented 6 months ago

Summary

The current equality check has received some dissatisfaction due to performance (and is a source of jank/missed frames in my application)
This is an unopinionated way to allow those consumers to provide their own equality function with better performance

Details

Fixes: https://github.com/zino-hofmann/graphql-flutter/issues/1315
Improves: https://github.com/zino-hofmann/graphql-flutter/issues/424
The default in this package is const DeepCollectionEquality().equals
You can now optionally provide your own (an example one is included based on this answer)
From my testing, writing a simple fragment with 10 other observables, it takes 150ms (on the main thread) vs 30ms (with the optimizedDeepEquals)

vincenzopalazzo commented 5 months ago

LGTM, I am wondering if you can work around about the test failure?

--------------------------------------------------------------------------------
example:
Formatted 3 files (0 changed) in 0.20 seconds.
Analyzing ....
No issues found!
example: SUCCESS
--------------------------------------------------------------------------------
graphql:
Formatted 55 files (0 changed) in 0.66 seconds.
Analyzing ....

   info - lib/src/cache/fragment.dart:8:8 - The import of 'package:gql_exec/gql_exec.dart' is unnecessary because all of the used elements are also provided by the import of 'package:graphql/client.dart'. Try removing the import directive. - unnecessary_import

1 issue found.

It is just a make fmt iirc

kvenn commented 5 months ago

Got it! Removed the unused important.

I also cleaned it up a little

Added the new helper function as an export
Left the default equality check as it is now so that this PR changes zero functionality, just adds customizability
Removed the commented out code.
Improved docs

Would you like me to document the new constructor argument? Or leave it as it is now with the global being documented?

RobertBrunhage commented 5 months ago

@vincenzopalazzo is this something we can get in so I don't have to use a forked version? :)

vincenzopalazzo commented 4 months ago

Ops I missed this for some reason, we can remove the merge commits @kvenn ?

Maybe just rebasing on main?

Thanks

RobertBrunhage commented 3 months ago

sorry for bumping this again @kvenn. Would be nice to do the above to get it in or if you are swamped I could make a PR

kvenn commented 3 months ago

I rebased and squashed all my commits together. Please let me know if there's anything else I can do! We've been using this fork for the past 2 months and it's been blazing fast!

Also, if you'd like to keep the git history clean, squash merging these types of feature branches seems like a good alternative.

kvenn commented 3 months ago

Done! Thanks for the reviews :)