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

Also attempt to exit early when iterating b's keys in shallowEqual #576

Closed
wants to merge 1 commit into from

Conversation

jcready
Copy link

@jcready jcready commented Dec 14, 2016

@jimbolla
Copy link
Contributor

Can you run some perf benchmarks just to be sure it's faster? You know that old saying about what happens when you ASSUME.

@appden
Copy link
Contributor

appden commented Dec 17, 2016

I'm more concerned of the correctness of shallowEqual in react-redux. There are a few subtle ways it could fail right now. I think it would be best to copy shallowEqual from fbjs.

@jcready
Copy link
Author

jcready commented Dec 17, 2016

I agree with @appden here. For instance shallowEqual in react-redux doesn't account for NaN. If an object contained any NaN value then shallowEqual would always return false.

@timdorr
Copy link
Member

timdorr commented Dec 29, 2016

I'm going to close this out and we can switch over to the fbjs version. It seems to cover more edge cases and might even be faster in common cases for react-redux.

@timdorr timdorr closed this Dec 29, 2016
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