Closed goce-cz closed 4 years ago
It looks like you've already made a pretty compelling case for changing the equality function to Object.is
in the issue description. I like the idea because if we consider that the comparisons are somehow ordered by complexity Object.is < shallowEqual < deepEqual
it makes sense to make the simplest one the default. The only doubting question then is, do we actually have valid use-cases where Object.is
suffices? If so, I would proceed with the change right away.
The only doubting question then is, do we actually have valid use-cases where Object.is suffices?
The Object.is
is IMO sufficient in 90% cases where you are just selecting sub-state. In case of a complex shape, updates usually happen the way that the previous object is just spread and only affected parts are replaced:
return {...prev, affected: newAffected}
so in case we are selecting via:
return state.nonAffected
we are still getting the same instance.
So yeah, I'm in favor of changing this to Object.is
.
I would still like to hear an opinion of @Bartunek and @Fezzzi as they are currently the only users of this hook (apart from me).
I would change the default to Object.is
as well. I agree with what @shkarpa said and it always feels natural to me in similar cases to expect the default be the simplest and most performant option.
I have no objections and agree with all of you 👍
I agree with... every one else. :) I think @Bartunek 's point especially is salient (it always feels natural to me in similar cases to expect the default be the simplest and most performant option), and I agree 100%
The
usePartialSnapshot
hook accepts equality function as an optional argument, the default value beingisShallowEqual
.While the default value makes the hook usable in most of the scenarios out of the box it has a couple of drawbacks:
Object.is
basically everywhere and at some places allows custom functions to be providedisShallowEqual
comparison in cases whereObject.is
would suffice is an overkillObject.is
as the equality function, majority of them won't do it as the default works wellObject.is
theisShallowEqual
has to traverse all the propertiesI'm kinda inclined to use
Object.is
as the default equality function and make the users reach forisShallowEqual
manually when needed.I'd love to hear some opinions here.