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

Adds check for valueOf/stringOf existence #93

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ctrlplusb
Copy link

@ctrlplusb ctrlplusb commented Jan 14, 2021

When utilising this in a React context I was getting exceptions where a.valueOf / a.toString did not exist. The current logic didn't take this into account, leading to an exception when it tried to execute said functions.

I patched my local copy of your package with this fix and it worked great thereafter.

Here are the benchmark results post this patch:

fast-deep-equal x 280,216 ops/sec ±0.56% (90 runs sampled)
fast-deep-equal/es6 x 237,314 ops/sec ±0.55% (85 runs sampled)
fast-equals x 258,718 ops/sec ±0.58% (89 runs sampled)
nano-equal x 201,614 ops/sec ±0.54% (89 runs sampled)
shallow-equal-fuzzy x 154,959 ops/sec ±0.77% (90 runs sampled)
underscore.isEqual x 97,879 ops/sec ±0.53% (91 runs sampled)
lodash.isEqual x 40,880 ops/sec ±0.60% (89 runs sampled)
deep-equal x 196 ops/sec ±1.42% (58 runs sampled)
deep-eql x 44,681 ops/sec ±0.63% (90 runs sampled)
ramda.equals x 13,237 ops/sec ±0.83% (89 runs sampled)
util.isDeepStrictEqual x 59,043 ops/sec ±0.55% (89 runs sampled)
assert.deepStrictEqual x 503 ops/sec ±0.56% (87 runs sampled)
The fastest is fast-deep-equal

@coveralls
Copy link

coveralls commented Jan 14, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling 193609e on ctrlplusb:master into a8e7172 on epoberezkin:master.

@darcyparker
Copy link

I can't approve this, but wanted to say I think this looks good and worth merging.

There are edge cases where a and b do not have Object.prototype in its prototype chain, and a.valueOf() and a.toString() may not be implemented.

@darcyparker
Copy link

One suggestion is to add some unit tests to capture the requirement/need to test for a.valueOf and a.toString.

ie create some objects like Object.assign(Object.create(null), {a: 1, b: 2}) (which does not have .valueOf or .toString)

@unbyte
Copy link

unbyte commented Sep 1, 2021

any progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants