Skip to content

shallowEqual: bail if either argument is falsey #3317

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

Merged
merged 1 commit into from
Mar 10, 2015

Conversation

ianobermiller
Copy link
Contributor

Also add some unit tests.

Fixes #3316

@jimfb
Copy link
Contributor

jimfb commented Mar 5, 2015

I like this PR, especially since it is predominantly adding unit tests. I'm going to mark it as accepted, but pinging @spicyj just in case I missed something (the semantics of javascript falsey expressions are subtle). We are in a code-freeze for the 0.13 release, so ETA for a merge is a week-ish.

@syranide
Copy link
Contributor

syranide commented Mar 5, 2015

This seems weird to me I think, why do we want this behavior?

@sophiebits
Copy link
Collaborator

@syranide Why not?

@jimfb
Copy link
Contributor

jimfb commented Mar 5, 2015

@syranide returning false (if one object is null and the other is not) seems more correct than throwing an unexpected exception.

@syranide
Copy link
Contributor

syranide commented Mar 6, 2015

@JSFB Ok, after reading it a bit more I'm kind of OK with it, but it's not super obvious, I'd say it could make more sense to make it objA == null || objB == null for clarity? shallowEqual(1, 2) === true. props and state can indeed be non-objects too, although perhaps that is being restricted.

jimfb added a commit that referenced this pull request Mar 10, 2015
shallowEqual: bail if either argument is falsey
@jimfb jimfb merged commit 91b4564 into facebook:master Mar 10, 2015
@jimfb jimfb mentioned this pull request Mar 10, 2015
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.

Calling setState without getInitialState throws when using PureRenderMixin
4 participants