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

Reimplement isEqual without pulling in massive lodash.isequal. #4924

Merged
merged 4 commits into from
Jun 6, 2019

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 5, 2019

Follow-up to #4915. I have kept the tests from that PR, just not the lodash-based implementation of isEqual.

The lodash.isequal package has a ton of extra features we don't need, and weighs in at a whopping 10KB minified (3.8KB after gzip). Very little of that machinery is really necessary to fix #4824.

I extracted this functionality into a separate npm package called @wry/equality, so we have the flexibility to depend on the same logic in other packages without importing apollo-utilities.

Here's the new implementation.

@benjamn
Copy link
Member Author

benjamn commented Jun 5, 2019

I was mistaken when I claimed, "Since the lodash.isequal package is already used by react-apollo, this change is likely to decrease total bundle size," because React Apollo will soon be removing lodash.isequal: apollographql/react-apollo@feb9b0c

@benjamn
Copy link
Member Author

benjamn commented Jun 5, 2019

@capaj In the process of reimplementing isEqual I did find a bug in lodash.isequal, which I thought you might find interesting because you advocated for not testing the lodash.isequal implementation:

const isEqual = require("lodash.isequal");

const a = { self: { self: { self: {}}}};
a.self.self.self.self = a;

const b = { self: {}};
b.self.self = b;

console.log(isEqual(a, b)); // true
console.log(isEqual(b, a)); // false

It would be fine for isEqual to return false in both cases, but returning different results when the arguments are reversed violates symmetry (one of the required properties of any equivalence relation).

@benjamn benjamn self-assigned this Jun 5, 2019
@benjamn benjamn requested a review from hwillson June 5, 2019 22:32
@capaj
Copy link
Contributor

capaj commented Jun 5, 2019

Very nice find. Did you open a bug in Lodash repo?

@benjamn
Copy link
Member Author

benjamn commented Jun 5, 2019

@capaj Planning to do that, though I think we'll stick with the smaller implementation. Thanks for sending me down this path!

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Great @benjamn!! 👍

Follow-up to #4915. I have kept the tests from that PR, just not the
lodash-based implementation of isEqual.

The lodash.isequal package has a ton of extra features we don't need, and
weighs in at a whopping 10KB minified (3.8KB after gzip). Very little of
that machinery is really necessary to fix #4824.

I extracted this functionality into a separate npm package called
@wry/equality, so that we have the flexibility to depend on the same logic
in other packages without importing apollo-utilities.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Call Stack Exceeded Error in isEqual
3 participants