use-context-selector icon indicating copy to clipboard operation
use-context-selector copied to clipboard

Support for custom equality function

Open robeady opened this issue 5 years ago • 47 comments

It seems like the library is currently hardcoded to compare the value produced by the selector using Object.is

https://github.com/dai-shi/use-context-selector/blob/ba7476affc7cf198c9ed72c2f5db538b3998ac39/src/index.js#L82

Sometimes it may be useful to use a different equality comparison such as shallow equal when composing an object. The redux useSelector API has an extra optional parameter for this purpose, which can be used like

const selectedData = useSelector(selectorReturningObject, shallowEqual)

Would you consider supporting this in use-context-selector?

robeady avatar Apr 29 '20 00:04 robeady

Hi, thanks for opening an issue. ~This might be controversial, but I have a strong belief that react community should prefer using useCallback to equalityFn. In the case of react-redux, it can't be helped because it had to provide a clear migration path from the HoC API.~ ~I'm pretty convinced about this with the upcoming useMutableSource. v2 is already implemented in #12, and it requires useCallback. Supporting equalityFn with useMutableSource would be extra work, and I'm suspicious about how to clearly make the implementation Concurrent Mode safe.~

~That said, I'm eager to learn the use case that requires equalityFn and provide a workaround or even a util function. Do you have an example that we can discuss on?~

(I was stupid back then..)

dai-shi avatar Apr 29 '20 00:04 dai-shi

@dai-shi I'm not sure I follow. If a selector composes an object, using referential equality will always trigger a rerender. Shallow equality would help; how would useCallback help instead?

victorporof avatar Apr 30 '20 06:04 victorporof

const selector = useCallback(s=>({a: s.a, b: s.b}),[])
const obj = useSelector(selector)

lishine avatar Apr 30 '20 06:04 lishine

@lishine The selector callback is cached, but it returns a new object every time. The selected value will always be different referentially.

victorporof avatar Apr 30 '20 06:04 victorporof

Ok, my bad, it actually never triggers with the above code. Then create this object in the useContextState with a useMemo. I mean where you create the state for the useSelector

lishine avatar Apr 30 '20 07:04 lishine

That would violate the rule of hooks, wouldn't it? Using "useMemo" in a selector means that it's called in function that is neither a component nor a custom hook.

I suppose the deeper underlying point is that you can memoize the returned object (not through useMemo, but something else like reselect or just rolling your own memoization), but that's just ultimately a roundabout way of doing custom equality checks.

victorporof avatar Apr 30 '20 07:04 victorporof

@victorporof Sorry, I think I mixed the two points. Ref equality is almost default with useMutableSource (and React in general), so supporting equalityFn is an extra work that I think shouldn't be part of this library. So, one would need to use memoization library before passing. I'm not very familiar, but reselect can be used or there would be more alternatives.

(Requiring useCallback is a different story.)

dai-shi avatar Apr 30 '20 07:04 dai-shi

To be clear, this is the point myself (and I believe OP) are referring to: https://react-redux.js.org/api/hooks#equality-comparisons-and-updates

If this library choses to only support memoization instead of equality checks for the generated selector data, then sure. However, the useCallback discussion seems unrelated.

victorporof avatar Apr 30 '20 07:04 victorporof

However, the useCallback discussion seems unrelated.

Right, that was my bad..

(It's just a bit related when one were to implement it.)

dai-shi avatar Apr 30 '20 07:04 dai-shi

So, one would need to use memoization library before passing.

This is what I meant. In the hook that you pass to the provider, wrap the object with useMemo.

lishine avatar Apr 30 '20 07:04 lishine

@lishine You can't useMemo inside a useCallback, or another function that isn't a hook or a function component.

victorporof avatar Apr 30 '20 07:04 victorporof

@lishine Yeah. Note that useMemo can only be used in render, and often the case with a selector is not in render.

dai-shi avatar Apr 30 '20 07:04 dai-shi

Sure in render , in the component that includes the provider.

lishine avatar Apr 30 '20 07:04 lishine

I write a custom hook that creates the state. That state is passed to the provider.

lishine avatar Apr 30 '20 07:04 lishine

I write a custom hook that creates the state. That state is passed to the provider.

lishine avatar Apr 30 '20 07:04 lishine

@lishine Are you talking about memoizing the whole state that is passed to the provider? That is different from from selecting multiple values from that state.

victorporof avatar Apr 30 '20 07:04 victorporof

If you want to return an object from the selector that includes several properties of the state, you create this object beforehand where you manage the state. And the select this property in the selector.

lishine avatar Apr 30 '20 07:04 lishine

@lishine Aha, I see what you mean. I think that's a reasonable workaround (just like many others, such as using reselect, or memoizing the returned value from a selector on the spot). I think we're in agreement that being constrained with just being able to use referential equality doesn't mean that there's no other ways of doing this.

I'd say that there can be various possible combinations of substate that the provider component would then have to know about, which means propagating these concerns up the component tree. I maintain that this workaround (and memoization inside selectors) are less ergonomic or natural than just being able to pass in a custom equality function in the selector. However I also concede that this might be because of some folks just being used to react-redux's approach.

victorporof avatar Apr 30 '20 07:04 victorporof

Note: doing this sort of memoization inside the provider component doesn't seem possible when it'd depend on various props that are available only to some deeply descendant component.

victorporof avatar Apr 30 '20 07:04 victorporof

some folks just being used to react-redux's approach.

In this context, my preference it to use multiple useSelectors.

dai-shi avatar Apr 30 '20 10:04 dai-shi

@dai-shi Multiple selectors aren't suitable in all circumstances. Consider this:

const stuff = useSelector(state => {
  const { foo, bar } = expensive(state, props.baz);
  return { foo, bar };
});

vs.

const foo = useSelector(state => expensive(state, props.baz).foo);
const bar = useSelector(state => expensive(state, props.baz).bar);

...where baz is a prop. Then also consider the situation where this is used in a component that is instantiated multiple times (e.g. a set of children where each baz is a different id).

Multiple selectors won't help. You want to build a memoized selector factory, with a memo'ed caching selector for each component instance. Which is fine (and what you'd do with reselect for example); it's just that shallow equality would be so much simpler, and one of the reasons why react-redux opted to support it in addition to allowing consumers roll up their own memoization strategies, instead of enforcing them.

victorporof avatar Apr 30 '20 10:04 victorporof

Thanks for a concrete example. Totally makes sense. (In my redux apps, I would do expensive work in reducers, and all selectors are just "selecting" parts of state. That's not always the case for everyone, though.)

Here's a rough idea. There might be some issues in CM. 🤔

const useSelectorWithEqlFn = (selector, eqlFn) => {
  const ref = useRef();
  const patchedSelector = useCallback((state) => {
    const selected = selector(state);
    if (eqlFn(ref.current, selected)) {
      return ref.current;
    }
    ref.current = selected;
    return selected;
  }, [selector, eqlFn]);
  return useSelector(patchedSelector);
};

dai-shi avatar Apr 30 '20 10:04 dai-shi

@dai-shi While that works fine, that would trigger a re-render wouldn't it? My point wasn't necessarily about receiving a memoized object for the consumer code, but making sure that the components don't rerender in the first place.

victorporof avatar Apr 30 '20 13:04 victorporof

Scratch that, I think that should work fine. You'd also need to be careful that selector is itself handled via useCallback. It's a mountain of hooks :)

victorporof avatar Apr 30 '20 13:04 victorporof

Wow this blew up :)

Yes @victorporof gave a reasonable example use case. Mine was pulling from multiple parts of the state, contingent on a null check. Here's a simplified example:

const foo = useSelector(s => s.flag && s.foos[fooId]);
const bar = useSelector(s => s.flag && s.bars[barId]);

which could be better replaced by

const fooAndBar = useSelector(
    s => s.flag && ({ foo: s.foos[fooId], bar: s.bars[barId] }), 
    shallowEqual
);

I use typescript and sadly the first version loses the proof that foo and bar are either both null or both present. And I don't want to push this projection into the state itself, because that would create duplication of data and consistency issues (I use this library like one is encouraged to use redux, with normalised immutable state).

The other obvious workaround is to use reselect and I think it's fair to recommend that instead :)

Especially if useMutableSource (which I'm not familiar with) isn't compatible with the idea of adding an equalityfn parameter.

robeady avatar Apr 30 '20 18:04 robeady

I haven't actually used reselect before - I looked at the documentation, and had some trouble understanding how to apply it to my example. Would it be like this?

const fooAndBarSelector = useMemo(
    () =>
        createSelector(
            s => s.flag,
            s => s.foos[fooId],
            s => s.bars[barId],
            (flag, foo, bar) => flag && { foo, bar },
        ),
    [fooId, barId],
)
const fooAndBar = useSelector(fooAndBarSelector)

I guess that works although it's quite finicky

robeady avatar Apr 30 '20 19:04 robeady

Or maybe this works? reselect docs have some room for improvement regarding extra arguments :)

// top level
const fooAndBarSelector = createSelector(
    s => s.flag,
    (s, { fooId }) => s.foos[fooId],
    (s, { barId }) => s.bars[barId],
    (flag, foo, bar) => flag && { foo, bar },
)
// in my component
const fooAndBar = useSelector(s => fooAndBarSelector(s, { fooId, barId }))

with the caveat that there has to be one fooAndBarSelector per instance of the component (luckily I have just one)

robeady avatar Apr 30 '20 19:04 robeady

I use typescript and sadly the first version loses the proof that foo and bar are either both null or both present.

I hadn't thought about this, but it's very convincing. Thanks for the use case.

Especially if useMutableSource (which I'm not familiar with) isn't compatible with the idea of adding an equalityfn parameter.

It's more like useRef has to be used carefully in Concurrent Mode.

reselect

You want to avoid accessing s.foos if the flag is falsy. So, maybe something like this?

const fooAndBarSelector = createSelector(
    s => s.flag,
    (s, { fooId }) => s.flag && s.foos[fooId],
    (s, { barId }) => s.flag && s.bars[barId],
    (flag, foo, bar) => flag && { foo, bar },
)

dai-shi avatar Apr 30 '20 23:04 dai-shi

You want to avoid accessing s.foos if the flag is falsy. So, maybe something like this?

Oh yes, that makes more sense, thank you

robeady avatar Apr 30 '20 23:04 robeady

To circle back around, are there any implications of concurrent mode and/or useMutableSource on the notion of introducing a custom equality function parameter?

Or, is that a separate discussion, and is the main issue with the custom equality function idea one of API design and steering usage in the right way?

robeady avatar Apr 30 '20 23:04 robeady