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

Use shallowEqual from fbjs #591

Merged
merged 1 commit into from Feb 17, 2017
Merged

Use shallowEqual from fbjs #591

merged 1 commit into from Feb 17, 2017

Conversation

timdorr
Copy link
Member

@timdorr timdorr commented Dec 29, 2016

Fixes #590

Also handles things like NaN.

@jimbolla
Copy link
Contributor

We should perf test this, since it's called so much. It's useful to remember that we're not comparing any 2 rando values, but specifically props objects coming from react. I think we can make certain assumptions, like that they're objects, and typically (99% of time?) that the # of props is going to be the same between the 2 args. So the algorithm can optimize for things like that.

@timdorr
Copy link
Member Author

timdorr commented Dec 29, 2016

True, but I think the effects are minor. Given this is a hot path, it's going to get optimized pretty heavily. I think the only thing to do is to use the for in loop form instead, as v8 has specific optimizations for that with a hasOwnProperty check. Though it probably won't trigger that with the equality check in there. Maybe if we unroll that into another if?

@jimbolla
Copy link
Contributor

If the perf of the fbjs version is fine, then maybe we should just import it from fbjs. fbjs is a dependency of react, so react-redux has a dependency on it indirectly already.

@timdorr
Copy link
Member Author

timdorr commented Feb 17, 2017

AFAIK, shallowEqual hasn't changed in quite some time in fbjs. I'm going to merge this as-is and we can look at switching it to an import at a later date.

@timdorr timdorr merged commit 573db0b into master Feb 17, 2017
@timdorr timdorr deleted the shallowEqual branch February 17, 2017 14:46
@jimbolla
Copy link
Contributor

Works for me. This should be fast enough for normal usage. And if someone really needs a faster implementation than the fbjs, they have the power to override it in the options.

@gaearon
Copy link
Contributor

gaearon commented Feb 17, 2017

I wouldn't recommend depending on anything from fbjs directly.

@gaearon
Copy link
Contributor

gaearon commented Feb 17, 2017

(Copying is fine 😄 )

@wtgtybhertgeghgtwtg
Copy link

This seems like it happens often; recompose just has fbjs as a dependency for shallow equal, and I think react-side-effect used to do that. Do you think fbjs could move their shallow equal implementation into another library? Copying doesn't seem like the best solution, either.

webguru7 pushed a commit to webguru7/react-redux that referenced this pull request Dec 8, 2022
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.

None yet

4 participants