-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
refactor to use lodash.isEqual instead of custom isEqual #4903
Conversation
Related issue: #4824 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding the dependency to apollo-cache-inmemory
and apollo-client
, let's just reimplement the apollo-utilities
version of isEqual
using lodash.isequal
.
@benjamn so the tests for |
I didn't say anything about tests in my previous comment, so I don't understand what you're objecting to. I see no reason to remove |
@benjamn do you really think having a utility function like this is beneficial to the apollo-client's codebase: https://github.com/capaj/apollo-client/blob/3e992f93328a94cdb5ef66795cef417d6df25698/packages/apollo-utilities/src/util/isEqual.ts#L6 I know you can't merge this and release it just with a simple patch bump. I am not asking you to. I just think wrapping lodash with an extra function is not desirable in any JS codebase. SO IMHO best would be to deprecate it, release a patch. Then remove it and release a new major with the function removed. |
Adding this to the 3.0 milestone so we can revisit whether |
I was trying out a hermes cache and ran into this issue: convoyinc/apollo-cache-hermes#414
now in order to solve this issue I think the best option is to support comparing cyclic objects in
isEqual
. Current custom implementation does not support them and runs into an infinite recursion.Thankfully lodash does support cyclic objects just fine and since it does mirror the behaviors of the current custom
isEqual
, I hope it could be used instead.lodash.isequal
package is already required by react-apollo so people using apollo-client in react won't see any bundle size change.The only other way of resolving the hermes cache issue I see could be to offer a way to let a cache override the
isDifferentFromLastResult
implementation, but sinceisEqual
is in the util package, IMHO it would be only a matter of time when someone would run into this problem in the future when reusing theisEqual
on other places in apollo monorepo.Checklist:
I also inadvertently made some formatting changes as I saved the files. Hope that it's okay-it's just some formatting.
NOTE that
apollo-utilities
package will need to be bumped by a major as I have removed the util function from the package.