Replace shallowEqual with reference equality in useSelector

[perrin4869]

I thought I'd put this out there to get discussion going on this, before 7.1 final goes out, as I think this is a very important thing to iron out, and having a PR implementing the change will go a long way.

As discussed in #1252 (comment), the argument for keeping shallowEqual seems to be that it is how connect currently works - well, as already stated in that thread, useSelector is not a replacement to connect, and the hooks API have already deviated from the familiar HOC API already, for example, dropping useAction.

I think that consumers of useSelector will be familiar with useState and will expect an API similar to that one - which deviated from setState in the same way that this PR deviates from connect. I think it will cause confusion for people when suddenly they see that their const username = useSelector(getUsername) hook causes rerenders they aren't expecting, and will have to scourge through the docs to find the very awkward looking workaround: const { username } = useSelector(state => ({ username: getUsername(state) })).

Additionally, the use of shallowEqual here seems to be a case of early optimization which could possibly be introduced in the future if the need arises.

[netlify]

Deploy preview for react-redux-docs ready!

Built with commit 062d5fc

https://deploy-preview-1288--react-redux-docs.netlify.com

[timdorr]

I'm not sure I agree with this approach. It puts too much onus on the user to memoize, without giving them the tools to do that. You can't useMemo, because state isn't (easily) available to make a dep. That makes getting good performance hard for end users and I think it should be really easy.

I feel like a better option is to make this customizable via an option, as was suggested here: #1252 (comment)

[MrWolfZ]

Yeah, if at all I would suggest making this an option.

[artem-malko]

@perrin4869 I'm just a passerby, but I'd like to know, how much === is faster than shallowEqual? Is it a real problem?

[perrin4869]

Hm... I am still a bit confused.
The official docs will then suggest to use an approach such as:

const {
  username,
  groups,
  todos,
} = useSelector(state => ({
  username: getUsername(state),
  groups: getGroups(state),
  todos: getTodos(state),
});

If so, then keeping shallowEqual as a default would make sense. However, in the redux docs, getUsername, getGroup and getTodos would all be single selectors, and yet the hook is useSelector, not plural. Maybe it should be renamed into useSelectors?

If the docs instead choose:

const username = useSelector(getUsername);
const groups = useSelector(getGroups);
const todos = useSelector(getTodos);

Then shallowEquals will really only optimize the single usecase where the selector returns a one-level deep object/array, which in the example above would probably apply to just groups being a supposed array of string, since username is a string, and todos an array of objects. As with connect, the need to memoize will exist no matter what

I am all for adding an option, but I think that this PR should be the default, and shallowEqual should be left to the user.

[timdorr]

While people should be breaking up their useSelector usage into multiple calls, we can't enforce that. And the practical reality is that many people are just going to copy-paste their existing connect args into Hooks.

While we can and should advise away from that style of usage, we can't prevent it and should use a reasonable default to prevent it from being a giant pain for most users.

[MrWolfZ]

An alternative to using a local option would be to make it a global one, either via the provider or just statically in code, something like useSelector.setEqualityComparer(myComparer).

Btw, my main argument for keeping shallow equality as the default is not that it makes it easier to migrate from connect, but that it makes it harder to shoot yourself in the foot. Even without making the comparer an option, it is trivial to work around it if your use case really requires it for the performance (which should really only be the case when you return an object with tens of thousands of keys from the selector):

const { myLargeObject } = useSelector(state => ({ myLargeObject: mySelector(state) }))

So with shallow equality as the default, both versions (returning an object or using multiple useSelector calls) work just fine, but with reference (or Object.is) equality, the object form will immediately lead to terrible performance, and it is quite tricky to work around this. As @timdorr said, the multi-hook approach may be more idiomatic, but should this library really be so opinionated as to enforce that pattern by punishing anything else?

[josepot]

Even without making the comparer an option, it is trivial to work around it if your use case really requires it for the performance (which should really only be the case when you return an object with tens of thousands of keys from the selector)

The problem is that the performance of this:

const { myLargeObject } = useSelector(state => ({ myLargeObject: mySelector(state) }))

Is actually pretty bad compared with the performance of doing referential equality. Because with referential equality the only thing that happens is a straight comparison, nothing else.

Compare the number of operations that take place with referential equality with the number of operations that will take place with the "trick" that you are suggesting:

• Every time that a dispatch takes place that selector will create a superfluous object.

• As a result of that, useSelector will detect a different object at every dispatch, so when shallowCompare evaluates the is function, that will return false every single time.

• Then, it will check if one of the objects is null, which they will never be.

• Then, it will get the keys of the previous objects and compare their length, which will always be the same

• And finally it will compare its only key and it will return true/false depending on that comparison.

All that is going to happen every single time that a dispatch takes place.

The thing is that if you have an already memoized selector that returns a large object, you will be better off not using the trick that you are suggesting.

Because without using the trick that you are suggesting, you will avoid the creation of superfluous objects at every dispatch and at least the initial comparison of the shallowCompare will always return true if the object has not changed. Only the few times that the object changes will you pay the cost of the "unnecessary" comparisons of its values. Let's keep in mind that the reason for memoizing a selector is that the selector is not going to be changing at every dispatch, because if it does then there is no point in memoizing it.

Long story short: the trick that you are suggesting looks smart, but in reality it would only make performance worse if you already have a memoized selector.

[MrWolfZ]

@josepot I think we are talking about different things. Let me outline my understanding of this issue from the beginning, so that you may point out where I am going wrong since I feel I must have fundamentally misunderstood something.

As far as I understand the main motivation behind not wanting to do a shallow equality comparison is that people don't want to pay the price for it if the selected slice of state is an object with many keys or a large array. The OP's post also mentions that const username = useSelector(getUsername) with shallow equality will cause unnecessary renders, which I don't really understand, since if getUsername just returns the same string it will always be evaluated as equal with the shallow comparison.

So, as you say, the shallow equality comparison does a reference equality check first. Therefore, if you are using a memoizing selector and don't use my workaround, then you won't feel the effects of the shallow comparison as long as the selected state doesn't change. In the cases where it does change, you will pay the price for the comparison. Now in regards to my suggestion it depends on how often the selected state changes. If it almost never changes, then you are probably fine with just sticking with the shallow comparison directly, since you rarely pay the full price for it. However, if your selected large array/object changes very often, then I suggest you use my workaround. The additional price you pay for those few operations you mention will then indeed be paid on each dispatch, but that price will be negligible in comparison to having to re-evaluate the selector in the first place.

Lastly, this is once again an issue about performance where people are making lots of assumptions. As I mentioned in another thread, when I prototyped the hooks API I ran the benchmarks (which have some worst case scenarios for shallow equality) with the shallow and reference equality comparison and saw no noticeable difference. If you want to convince us this change is necessary, please show us the need with concrete numbers and examples instead of hypotheticals.

[josepot]

@MrWolfZ:

As far as I understand the main motivation behind not wanting to do a shallow equality comparison is that people don't want to pay the price for it if the selected slice of state is an object with many keys or a large array. The OP's post also mentions that const username = useSelector(getUsername) with shallow equality will cause unnecessary renders, which I don't really understand, since if getUsername just returns the same string it will always be evaluated as equal with the shallow comparison.

I can't talk for others. My main reasons for not wanting to do a shallow equality comparison inside useSelector are: correctness and that I would like to avoid creating a leaky abstraction.

If useSelector just performs strict-equality, then everything works correctly. It is easy to document, it is easy to explain what useSelector does and if someone actually has a performance issue because their selector is returning a new object every time. They can easily fix that performance issue in a thousand different ways: using a library like Reselect, using an enhanced version of useSelector, memoizing their object selector themselves, breaking it down into differrent useSelectors, etc. For instance, if some users actually needed a selector hook that has shallow-equality baked in, then we could add another hook like this one:

const getShallowMemoizedSelector = selector => {
  let latestResult;
  return state => {
    const result = selector(state);
    if (shallowEqual(result, latestResult) {
      return latestResult;
    }
    return (latestlResult = result);
  }
}

const useObjectSelector = selector => {
  const memoizedSelector = useMemo(
    () => getShallowMemoizedSelector(selector),
    [selector]
  );
  return useSelector(memoizedSelector);
}

Which is correct and it doesn't leak anything.

On the other hand if useSelector performs a shallow compare, then the user needs to know about that implementation detail, it has to be documented and it's something not very obvious. If a user forgets about that, then they will face situations where useSelector doesn't work as expected. IMO that's happening because we are giving up correctness in favor of a premature performance optimization.

That's why I think that if useSelector only performs strict equality, then we have a better primitive. Because it's possible to have the exact same behavior that the current useSelector has without sacrificing correctness or performance. However, as I already explained before, you can't create a useSelector that behaves and performs like the primitive that I would like to have from a useSelector that it's implemented using shallow-comparison.

Lastly, this is once again an issue about performance where people are making lots of assumptions. As I mentioned in another thread, when I prototyped the hooks API I ran the benchmarks (which have some worst case scenarios for shallow equality) with the shallow and reference equality comparison and saw no noticeable difference. If you want to convince us this change is necessary, please show us the need with concrete numbers and examples instead of hypotheticals.

I hope it's now clear that my main concerns are not "performance", but correctness and not to leak an implementation detail. However, I don't understand your reasoning with this comment. IMO you should be the one showing with numbers that this performance optimization is actually worth it. Meaning, that you should provide the numbers that show that this perf-optimization is needed, not the other way around.

@timdorr :

Almost a month ago I also thought and proposed that it would be a good idea to have this as an option of useSelector. However, I have changed my mind. The reason being that I would like to avoid opening the door to configuration parameters and overloads on these hooks. I think that it's better to have a solid primitive that does one thing and it does it well. I mean: if we start adding options, we may end up with a heavily overloaded hook and I would like to avoid having another case like connect. I would much rather to provide another hook instead, like the one that I proposed above.

Although, if this ends up becoming an option I think that the default option should be strict-equality 🙂

[markerikson]

Still busy, still skimming the discussions vaguely, but I'm also starting to get a bit tired of the back and forth here.

If people feel there are perf concerns, fork, benchmark, and PR.

[josepot]

@markerikson :

Still busy, still skimming the discussions vaguely, but I'm also starting to get a bit tired of the back and forth here.

If people feel there are perf concerns, fork, benchmark, and PR.

If you read my latest comment you will see that for some of us the main concern here is not performance. It's to avoid having an API that leaks an implementation detail and to have a hook that always works correctly.

Also, if later we want to change this, it won't be as easy as:

fork, benchmark, and PR

If 7.1 gets published with the current implementation, then if we wanted to change this we would need a major version upgrade, because this change wouldn't be backwards compatible.

I think that this is important. Please, take your time to better understand what all the different concerns are with the current implementation.

[markerikson]

@josepot : can you clarify what you mean by "leaking an implementation detail"? The equality approach used is going to matter regardless of what we choose. Folks will need to know that and write their code accordingly.

[josepot]

@josepot : can you clarify what you mean by "leaking an implementation detail"? The equality approach used is going to matter regardless of what we choose. Folks will need to know that and write their code accordingly.

@markerikson :

Using shallow-compare is a performance optimization. Performance optimizations are implementation details. Implementation details shouldn't be leaked through the API, and the users of the API should not have to work around an implementation detail in order to make their selectors work properly.

If useSelector by contract could only return plain JS objects, then it would be totally cool to perform a shallow-compare. Because that performance optimization would not leak through the API, it would be a completely transparent performance optimization.

However, that is not the case: useSelector can return anything. And that is cool, it will allow us to do things that we couldn't before.

A user can have a store that only contains serializable data. However, when deriving the state, they may want to compute some non-serializable data, like a Date, a Promise, a Map, a Set, etc. That is a very legit thing to do in selectors.

If useSelector can return anything, then performing a shallow-compare on its result that only works properly for primitives and plain JavaScript objects makes that implementation incorrect for some use-cases and it forces developers to work around that implementation detail.

Since useSelector can return anything, the correct way to detect when the result of a selector has changed is strict equality. That implementation wouldn't force anyone to have to work around an implementation detail.

If we find users that experience perf issues because their selectors return objects, then we can help them fix those perf issues in many different ways:

• They can use connect
• They can break it down into smaller useSelector
• We could provide a hook that's only meant for returning plain JS objects.
• They can use Reselect

That is, of course, if that performance issue actually exists, which we don't even know if that's the case. But I agree that's possible and even likely. However, that's something that can and should be solved outside of useSelector. Also, addressing that wouldn't imply a major version upgrade.

[markerikson]

The "performance optimization" part is critical. One of the key goals of React-Redux from day 1 has been that "your own component only re-renders when needed". If we didn't take care of that, you'd end up with every React component in the app re-rendering on every dispatch, which would kill performance. And that's the whole point here. The goal of this equality check is to ensure that this individual subscription only causes a re-render when whatever value(s) it's returning have actually changed.

I'm not ruling out strict equality as an option, I'm just saying that the way you're arguing about this (both terminology and insistence) isn't exactly selling it to me.

In general, the things we need to consider here are:

• What approach is going to lead folks to the best performance patterns by default?
• What approach will be easiest for people to use writing new code?
• What approach makes the most sense as "hooks-native"?
• What approach will be easiest to use when migrating from connect to hooks?

I don't have hard answers for any of those yet, but those should be some of the deciding factors.

[markerikson]

Let me phrase this another way:

Please show me some concrete cases where things break with the shallow-equality approach, and work "as expected" with strict equality.

The getUsername example from the start of this issue doesn't seem to qualify, because the first step of a shallow equality comparison is always to check if the two values are indeed ===.

[josepot]

Please show me some concrete cases where things break with the shallow-equality approach, and work "as expected" with strict equality.

@markerikson

I already did that in my previous comment when I shared with you this example, that it's based on a real case that you found on twitter.

A useSelector that returns a Promise won't detect the next promise because it would be shallowly equal to the previous Promise... I wouldn't store a promise on the redux-state, but I would derive a promise from the state, for sure. I could give you many more examples... There are lots of instances when it's required to derive Maps, Sets, Promises, etc from the state.

Let me prepare a few Codesandboxes for you. Would that actually help my case? Because, quite honestly, I feel like giving up here.

[MrWolfZ]

I am starting to come around to the reference equality side. The argument about selectors being able to return anything is compelling and something that I hadn't considered before. With connect you would never do that, but with useSelector that is absolutely possible.

I still see the arguments for the shallow comparison, but I am about 50/50 now.

If we decide to go with reference equality, we just need to make sure to very clearly state in the docs that either multiple useSelector calls should be used to fetch multiple pieces of state, or that a selector returning a new object every time needs to be memoized.

[markerikson]

Sure, more examples would help.

The flip side of this is that anyone who does want to return multiple values is going to have to self-memoize their selector. That may not be too difficult, but I would argue that returning an object full of values would be a lot more common than returning a Promise from a selector. It also would require anyone migrating from connect() to likely have to significantly rewrite their selector logic.

And, as @MrWolfZ pointed out, the workaround for wanting to return something else is trivial - just return the value you want in an object or array, and destructure the result.

[MrWolfZ]

Btw, one additional thing to consider should be this: if we go with reference equality, it is easily possible that the users will use object selectors everywhere without noticing the performance issues, since everything will still work correctly (or they may notice the issues very late, causing them to have to make big refactorings). If we go with shallow equality, then yes, the cases pointed out by @josepot will fail, but that will be quite obvious since the component won't update when the user expects it to. The workaround would IMO still be what I proposed above, i.e. wrapping the return value with an object. That should work for promises, maps, and any other value. However, this would then also need to be mentioned in the docs.

Sadly neither solution is perfect, so I am still at 50/50.

[dai-shi]

if we go with reference equality, it is easily possible that the users will use object selectors everywhere without noticing the performance issues, since everything will still work correctly (or they may notice the issues very late, causing them to have to make big refactorings).

Here's my two cents. I'm on the ref equality side.
What if we run a selector twice, check ref equality only in DEV mode and warn developers about it? This is the same idea from <StrictMode> in React.

[josepot]

More concrete cases where things break with the shallow-equality approach, and work with strict equality:

• Selectors that compute Dates from a time-stamp stored in the state (I've had many of those)

shallowEqual(new Date(123456), new Date(654321)) // => true

• Selectors that compute URLs from the state (I've had a few of those).

shallowEqual(
  new URL('https://github.com/reduxjs/react-redux/'),
  new URL('https://github.com/reduxjs/react-redux/pull/1288')
) // => true

• Selectors that compute URLs searchParams.

shallowEqual(
  new URL('https://github.com/reduxjs/react-redux?foo=foo').searchParams,
  new URL('https://github.com/reduxjs/react-redux/pull/1288?bar=bar').searchParams
) // => true

• Selectors that compute Sets from the state.

shallowEqual(new Set([1,2,3]), new Set([3, 2, 1])) // => true

• Selectors that compute Maps from the state.

shallowEqual(new Map([[1, 2], [2, 1]]), new Map([[1, 3], [2, 4]])) // => true

• Selectors that compute Promises from the state, like the one I already showed.

shallowEqual(Promise.resolve(true), Promise.resolve(false)) // => true

I'm sure that I could write a much longer list, but I will leave it here for now.

[markerikson]

@josepot : thanks for that list. So, in general, it seems to boil down to "instances of classes", pretty much.

I'm still not completely convinced, yet, but having that list does help as food for thought.

Overall, it seems like the options are:

• Use reference equality: forces people to memoize, and makes porting existing code more difficult
• Use shallow equality: makes it more difficult to return class instances, although destructuring works around that
• Ship two different hooks: adds to API surface area, probably unnecessarily
• Ship one hook with a named option (like useSelector(selector, {referenceEqual: true}): ugly
• Ship one hook with configurable equality (like useSelector(selector, (a, b) => a === b): also kinda ugly, but maybe less so? Would also perhaps allow for cases where someone wants to use something like _.isEqual(). Also has a parallel to React.memo(), maybe?

[markerikson]

So here's an idea :

• Reference equality by default
• Optional comparison function at the second arg
• Actually export our shallowEqual function to make it easy to pass in as the comparison

Thoughts?

[josepot]

@markerikson

I see it in a slightly different way.

IMO the React hooks API has been purposely designed so that if users want to move to hooks, they are in a way forced to break down the logic that they had in their classes into a lot smaller pieces. An example of that is how useState does not automatically merge update objects, like the class setState does. So, IMO hooks are designed in a way that invites users to be more modular.

That's why I don't necessarily think that it's a bad thing that porting existing code to hooks "invites" users to break their containers down into smaller pieces. I think that you will agree with me that even before hooks, it was a best practice to have many "lean" containers as opposed of having a few "fat" containers.

I think that in reality, the transition to hooks will be fairly easy for those that had "lean" containers and not so easy for those that had "fat" containers (which usually implies having lots of props). Just like the transition to hooks has been a walk in the park for those that were enhancing our components with tools like recompose, and it's going to be a nightmare for those who were writing tightly coupled classes with lots of logic in them...

I don't think that reference equality forces ppl to memoize, I think that it "invites" them to write leaner containers.

As I was finishing to write this comment I saw your latest comment. I like a lot that suggestion.

[josepot]

Hi @perrin4869 and @markerikson !

I really like @markerikson latest proposal. I think that it finds a very good balance between "correctness" and convenience. That's why I just opened the following PR in @perrin4869 repo, so that if @perrin4869 accepts it that commit will be added to this PR. If that makes things too complicated, no worries @perrin4869 just take my code and add your own commit.

The reasons why I like @markerikson idea so much is because:

• The default behavior is always "correct".

• I think that it addresses @MrWolfZ concern:

it is easily possible that the users will use object selectors everywhere without noticing the performance issues, since everything will still work correctly (or they may notice the issues very late, causing them to have to make big refactorings)

With this change it would be pretty easy for those users to fix those perf issues without making the refactor too tedious, just adding the second argument into the problematic useSelectors would fix the issue.

• It becomes trivial to create a useObjectSelector hook (if someone likes that, of course).

• Finally a library is exporting shallowEqual! 🎉

[timdorr]

I'm with the second arg being for equality, particularly because it mirrors React.memo. However, I'm really strongly in favor of shallowEqual being the default.

The vast majority of our users are dealing with serializable data in their stores and the selection of that data rarely goes through anything more than a simple field selection. So, referential equality is going to be the wrong choice for the majority of our users and will be a painful transition. This is evidenced by the relatively minimal usage of areStatesEqual in connect (3K usages out of 1M usages on GitHub public code).

Again, I want to see this be an option; customizability is good. But I don't think it's doing anyone any favors to force "good form" on everyone when they're not actually doing anything wrong.

[josepot]

@markerikson

@josepot : you keep using the word "correctness". Can you clarify exactly what you mean when you say that?

Sure!

In this specific case I mean that useSelector should work correctly for all cases and not just for the ones that we think that are the "most common ones".

If the documentation states that the selector of useSelector:

may return any value as a result, not just an object

and its definition is:

const result : any = useSelector(selector : Function)

Then, I think that the correct behavior is that it gets updated whenever the selector returns a different value. From the user point of view it shouldn't matter if that type is a number, a Date, a Promise, an Object... It should perform updates whenever any kind of value changes. That's what I would consider to be correct.

I think that defaulting the equality comparison function to something that doesn't work correctly for all cases (when that would be possible) implies sacrificing correctness for performance... And I think that there is no need for doing that.

I still think that if we wanted to help users migrate from connect to hooks, the best would be to provide them with a useObjectSelector hook that is optimized for selectors that return objects. Then we would have correctness and performance for everyone 🙂.

Would that increase the API surface area? Yes, it would. But so it does exporting shallowEquals and adding another argument to useSelector.

If useSelector did just one thing and it did it well (correctly), then we wouldn't need an extra argument that takes an equality comparison function. Because that hook could be used to create any other "selector hook" that performs any other kind of equality comparison via composition.

That would still be my preferred option. But I'm happy to compromise with a solution that at least allows for any value to work in the way that I consider to be correct.

Also, I'm aware that a point could be made by saying that if the docs stated that the selectors of useSelect are only allowed to return serializable objects, then we would already have correctness with the current implementation 🤷‍♂️. I guess that's another way to look at it. It would feel like cheating to me, but I guess that's an option. I say that it would feel like cheating because IMO that would be designing the API after an implementation detail. I would be curious to see how the typings of the definition end-up looking, though...

const result : anySerializableType = useSelector(selector : Function)

😅

PS: Thanks for asking for clarification. Regardless of whether you agree with my thoughts or not, that shows that you actually care about everyone's opinion. Even the opinions of those who are as annoying as I can be 😄. I really appreciate that.

[markerikson]

The issue isn't "serializability", per se - it's that most of the built-in JS class types don't have any instance fields, so multiple instances would be considered shallow equal. If I had two instances of a Person data class, odds are they'd have different this.name fields, and so they'd be considered not shallow equal.

I generally see what you're trying to say about "correctness". I don't feel it's nearly as big a deal as you're making it out to be, but I do understand what you're getting at. For that matter, someone could also argue that even doing equality-based comparisons itself is over-opinionated - what if someone were updating a Map instance in state directly, and yet want to have the UI re-render? That's not how Redux is supposed to be used, but someone could say that's a potential use case, and therefore having equality-based comparisons is too limiting.

I do disagree that you could "use this hook to create any other selector hook". That's sort of the root of the issue. The useSelector hook itself is already doing the update check internally and triggering the re-render. That's not something that's composable with other wrapping hooks. And that's kind of the point - it is its own primitive, much like useMemo() or useState(). It's "read the current data from the store as we're rendering", and "check to see if the extracted data has changed when an action was dispatched".

With connect(), we deliberately hid access to the store. You could always grab it off of old/new context, but that was unsupported. In theory, now that we're shipping useStore() to provide an "official" way to access the store in a component, anyone could go ahead and write their own more complex subscription hook if they really want to.

Given that we're having this debate over equality methods, I'd rather just make it configurable instead of exporting multiple hooks. The rest of the logic for subscriptions and everything is identical - there's no reason to duplicate that. It also opens up the door for someone to use _.isEqual or something like Immutable.js's comparison methods, if they want to.

[markerikson]

Awright. Semi-executive decision: let's go merge in perrin4869#1 and go with the plan I suggested.

I know @timdorr expressed a strong preference for shallow equality instead. That said, I'm willing to give the "ref equality + comparison func" approach a shot and see how it works out.

[josepot]

@markerikson

For that matter, someone could also argue that even doing equality-based comparisons itself is over-opinionated - what if someone were updating a Map instance in state directly, and yet want to have the UI re-render? That's not how Redux is supposed to be used, but someone could say that's a potential use case, and therefore having equality-based comparisons is too limiting.

Then they should go back to MVC where controllers can "poke" the models as much as they want 😄. I mean, I know that Redux is pretty unopinionated, but if you have chosen Redux is because you appreciate the benefits of working with an immutable state, and in JS what determines if an instance has changed is referential equality. There are "tricks" that can be made -in some instances- to determine if the new state is equivalent to the previous one, and in a FP paradigm if you can be certain that the next state is equivalent to the previous one, you can assume that nothing has changed, for sure! But those "tricks" only work in some cases.

I do disagree that you could "use this hook to create any other selector hook". That's sort of the root of the issue. The useSelector hook itself is already doing the update check internally and triggering the re-render. That's not something that's composable with other wrapping hooks. And that's kind of the point - it is its own primitive, much like useMemo() or useState(). It's "read the current data from the store as we're rendering", and "check to see if the extracted data has changed when an action was dispatched".

The beauty of having the guarantee that you are working with pure functions is that you can enhance them, so it is possible to do that by enhancing the selector.

Check this out, imagine that useStrictEqualsSelector is the hook that I would like to have as useSelector. With that hook, I could build a hook that is equivalent to the useSelector that is about to get merged, without repeated updates or anything:

const selectorEnhancer = (selector, equalityFn) => {
  let latestResult;
  return state => {
    const result = selector(state);
    if (equalityFn(result, latestResult) {
      return latestResult;
    }
    return (latestlResult = result);
  }
}

const useSelector = (selector, equalityFn) => {
  const enhancedSelector = useMemo(
    () => equalityFn
      ? selectorEnhancer(selector, equalityFn)
      : selector,
    [selector, equalityFn]
  );
  return useStrictEqualsSelector(enhancedSelector);
}

Given that we're having this debate over equality methods, I'd rather just make it configurable instead of exporting multiple hooks. The rest of the logic for subscriptions and everything is identical - there's no reason to duplicate that. It also opens up the door for someone to use _.isEqual or something like Immutable.js's comparison methods, if they want to.

Sure. That makes sense.

[markerikson]

Went ahead and pushed @josepot 's commit to @perrin4869 's branch rather than waiting for that sub-PR to be merged. Merging this in.

[perrin4869]

Given that we're having this debate over equality methods, I'd rather just make it configurable instead of exporting multiple hooks.

Just to clarify, the proposition here is not either or. I want to make useSelector configurable and expose a useObjectSelector which uses the new option. It's a one line implementation:

const useObjectSelector = selector => useSelector(selector, shallowEqual);

The reason is that we are exporting shallowEqual right now with a very specific use case in mind, and that is so that people can use it as a second option to useSelector. We might as well go ahead and implement it for them ;)

Exporting shallowEqual already extends the API as it is, and therefore must be documented, etc, with a single use case in mind. If we are extending the API and documenting it, why not do that with a new hook instead?

[markerikson]

Already merged :)

I don't want to ship a hook that's 99% duplicate, and I can see use cases for making the equality configurable.

I am throwing useShallowEqualSelector() into the hooks docs as a copy-pastable recipe.

[perrin4869]

Already merged :)

Yeah, thanks about that :)

and I can see use cases for making the equality configurable

I can too, I'm NOT saying to make it non configurable lol. Keep it configurable AND export a specific configuration. Just wanted to make sure it is not misunderstood

Well I'm just a bit iffy about exporting shallowEqual from this lib, but I can live with that.

[markerikson]

Well, it's still an alpha :) We can tweak things further if so desired.

[perrin4869]

Ok I'll send a PR tonight for your consideration then :)

…

On Mon, May 20, 2019, 10:34 AM Mark Erikson ***@***.***> wrote: Well, it's still an alpha :) We can tweak things further if so desired. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1288?email_source=notifications&email_token=ABMB27GLDPETVQOZQSZHUEDPWH52LA5CNFSM4HNWF4R2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODVXPUKA#issuecomment-493812264>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABMB27EOALXYWJIQLSMQ6N3PWH52LANCNFSM4HNWF4RQ> .

[tlrobinson]

I'm late to the party, but strongly agree with the decision to default useSelector to reference equality. To me, a "selector" often refers to the thing you use to a get a single prop in mapStateToProps, and since connect does a shallow equality check on mapStateToProps it's effectively doing reference equality checks on individual selectors.

It might make sense to have useSelector (reference equality) and useSelectors (shallow equality, essentially takes a mapStateToProps), but if those names are too similar then something like useObjectSelector works too.