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

Support for custom equality function #19

Open
robeady opened this issue Apr 29, 2020 · 47 comments
Open

Support for custom equality function #19

robeady opened this issue Apr 29, 2020 · 47 comments

Comments

@robeady
Copy link

robeady commented Apr 29, 2020

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

|| Object.is(ref.current.s, ref.current.f(nextValue))) {

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?

@dai-shi
Copy link
Owner

dai-shi commented Apr 29, 2020

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..)

@victorporof
Copy link

@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?

@lishine
Copy link

lishine commented Apr 30, 2020

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

@victorporof
Copy link

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

@lishine
Copy link

lishine commented Apr 30, 2020

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

@victorporof
Copy link

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.

@dai-shi
Copy link
Owner

dai-shi commented Apr 30, 2020

@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.)

@victorporof
Copy link

victorporof commented Apr 30, 2020

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.

@dai-shi
Copy link
Owner

dai-shi commented Apr 30, 2020

However, the useCallback discussion seems unrelated.

Right, that was my bad..

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

@lishine
Copy link

lishine commented Apr 30, 2020

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.

@victorporof
Copy link

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

@dai-shi
Copy link
Owner

dai-shi commented Apr 30, 2020

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

@lishine
Copy link

lishine commented Apr 30, 2020

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

@lishine
Copy link

lishine commented Apr 30, 2020

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

1 similar comment
@lishine
Copy link

lishine commented Apr 30, 2020

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

@victorporof
Copy link

@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.

@lishine
Copy link

lishine commented Apr 30, 2020

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.

@victorporof
Copy link

@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
Copy link

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.

@dai-shi
Copy link
Owner

dai-shi commented Apr 30, 2020

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

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

@victorporof
Copy link

victorporof commented Apr 30, 2020

@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.

@dai-shi
Copy link
Owner

dai-shi commented Apr 30, 2020

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);
};

@victorporof
Copy link

@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
Copy link

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 :)

@robeady
Copy link
Author

robeady commented Apr 30, 2020

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
Copy link
Author

robeady commented Apr 30, 2020

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
Copy link
Author

robeady commented Apr 30, 2020

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)

@dai-shi
Copy link
Owner

dai-shi commented Apr 30, 2020

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 },
)

@robeady
Copy link
Author

robeady commented Apr 30, 2020

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
Copy link
Author

robeady commented Apr 30, 2020

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?

@dai-shi
Copy link
Owner

dai-shi commented May 1, 2020

Besides my preference on API design, useRef is very difficult to use in CM. Looking back useSelectorWithEqlFn in #19 (comment) , I hope it works in CM, but not 100% confident.

Some references:

@robeady
Copy link
Author

robeady commented May 1, 2020

Reading those links there doesn't seem to be a place for this custom equality function idea with useMutableSource. But I found the API highly confusing. I was particular confused by the redux example

function Example() {
  // The user-provided selector should be memoized with useCallback.
  // This will prevent unnecessary re-subscriptions each update.
  // This selector can also use e.g. props values if needed.
  const memoizedSelector = useCallback(state => state.users, []);
  
  // The Redux hook will connect user code to useMutableSource.
  const users = useSelector(memoizedSelector);

that doesn't look very ergonomic or similar to how anybody uses redux selectors today... am I missing something here?

@dai-shi
Copy link
Owner

dai-shi commented May 1, 2020

useSelectorWithEqlFn creates memoized selector (and you need to pass stable selector and eqlFn).

I'm not sure if I understand your point. (and I'm not sure if I responded well to your concern.)

BTW, if you are interested in a full implementation with useMutableSource for Redux. I developed this dai-shi/reactive-react-redux#48 . The API is not backward compatible and more like proposing a new pattern.

@dai-shi
Copy link
Owner

dai-shi commented Jan 25, 2021

Okay, I may need to change my mind if React supports equalityFn. ref: facebook/react#20646
If it lands, we don't need useMutableSource, and there's no restriction.

@dai-shi dai-shi mentioned this issue Jan 25, 2021
36 tasks
@robeady
Copy link
Author

robeady commented Jan 25, 2021

I was thinking about this again recently, actually. I have seen more situations since I opened the issue where equalityFn would be useful, I don't remember specific examples right now but I think they are similar to the above, typically whenever you combine an inline non-trivial selector function with typescript type narrowing.

@dai-shi
Copy link
Owner

dai-shi commented Jan 26, 2021

It's still doubtful, if react really supports equalityFn. There's probably going to be a big debate.

For now, we should focus on creating a wrapper.
Rewriting #19 (comment)

import { useContextSelector } from 'use-context-selector';

export const useSelectedContext = (ctx, selector, eqlFn) => {
  const patchedSelector = useMemo(() => {
    let prevValue = null;
    return (state) => {
      const nextValue = selector(state);
      if (eqlFn(prevValue, nextValuue)) {
        return prevValue;
      }
      return (prevValue = nextValue);
    }
  }, [ctx, selector, eqlFn]);
  return useContextSelector(ctx, patchedSelector);
};

@johnrom
Copy link

johnrom commented Mar 12, 2021

patchedSelector, as a hook

https://gist.github.com/johnrom/4e8bc65110c689006663c7736539e892

@johnrom
Copy link

johnrom commented Mar 14, 2021

patched selector as a package https://www.npmjs.com/package/use-optimized-selector

@rhyek
Copy link

rhyek commented Apr 21, 2021

Hopefully this wrapper solution or something similar can be considered as the standard export of the package. I was really hoping to replace Redux with Context API for application state management, however while use-context-selector seemed promising in this endeavor, not being able to specify an equality function like you can in Redux is a deal-breaker.

The equality function option would be especially useful when replacing Redux with Context since you usually need to grab both values and methods (mutations) from your context object and so referential equality is a clear problem. With Redux you're doubly covered in this regard, because instead of selecting mutations you dispatch them through a separate mechanism.

Having the wrapper function mentioned in the above comment as a work-around is great, but I'm unsure how stable the solution is.

@dai-shi
Copy link
Owner

dai-shi commented Apr 21, 2021

While I understand where you are coming from, I'm still not confident if the equalityFn is going to be a mainstream in the React core. I'm not sure either how we can make it CM friendly. My hope is to do some experiment against a new experimental build of React and learn how it behaves (but that doesn't ensure anything for the future React versions.)

We had some discussions above and some of them are convincing the need of equalityFn. But, if it's about methods or Redux dispatch, I'd suggest to use two hooks.

const foo = useContextSelector(v => v.state.foo);
const dispatch = useContextSelector(v => v.dispatch);

Having the wrapper function mentioned in the above comment as a work-around is great, but I'm unsure how stable the solution is.

It should be just fine at this point. Not sure with concurrent mode thing in the future, but I guess your concern is for the future React too? So, including the workaround in the lib doesn't seem to help, does it?

@timbicker
Copy link

It's still doubtful, if react really supports equalityFn. There's probably going to be a big debate.

For now, we should focus on creating a wrapper.
Rewriting #19 (comment)

import { useContextSelector } from 'use-context-selector';

export const useSelectedContext = (ctx, selector, eqlFn) => {
  const patchedSelector = useMemo(() => {
    let prevValue = null;
    return (state) => {
      const nextValue = selector(state);
      if (eqlFn(prevValue, nextValuue)) {
        return prevValue;
      }
      return (prevValue = nextValue);
    }
  }, [ctx, selector, eqlFn]);
  return useContextSelector(ctx, patchedSelector);
};

Thanks for this nice hook. I observed a problem that I want to share, maybe it will help others too.

If you intend to use this hook like this for example:

function useSelectedArray(keys: string[]): string[] {
  return useSelectedContext(formStateContext,
    (state) => keys.map(key => state[key]),
    (prev, next) => deepCompare(prev, next))
}

The hook will never return the cached value but always a new value. The problem lies in the dependency of the useMemo hook which is: [ctx, selector, eqlFn]. In the above example, there is always passed a new selector and eqlFn.

To fix this, I edited the dependency array as follows []. Because in my code I know, the dependencies will never change..

Another option would probably be to use a useCallback hook for the the selector and eqlFn of useSelectedArray. But somehow this felt weird to me. What are your thoughts on this?

@dai-shi
Copy link
Owner

dai-shi commented Aug 23, 2021

@timbicker Hi, you understand it correctly. My expectation is to use useCallback, or what's better is to define those functions outside render whenever possible.
Now, I know this is controversial, but from library perspective, giving stable function from the library consumer is almost necessary.
We have a discussion thread about this for React 18: reactwg/react-18#84

@timbicker
Copy link

Alright, thanks!

@quolpr
Copy link

quolpr commented Sep 27, 2021

Here is the typescript version:

export const useEqlContextSelector = <T extends any, R extends any>(
  ctx: Context<T>,
  selector: (val: T) => R,
  isEql: (a: R | null, b: R) => boolean
) => {
  const patchedSelector = useMemo(() => {
    let prevValue: R | null = null;

    return (state: T) => {
      const nextValue: R = selector(state);

      if (prevValue !== null && isEql(prevValue, nextValue)) {
        return prevValue;
      }

      prevValue = nextValue;

      return nextValue;
    };
  }, [isEql, selector]);

  return useContextSelector(ctx, patchedSelector);
};

@valen214
Copy link

valen214 commented Aug 5, 2022

I don't think useMemo is need at all, in fact, it's doing the opposite becuase selector and eqlFn are most likely being updated every render, I believe useRef would best serve the purose (storing prevValue)

import { useContextSelector } from 'use-context-selector';

export const useSelectedContext = (ctx, selector, eqlFn) => {
  const prevValue = useRef(null);
  const patchedSelector = (state) => {
      const nextValue = selector(state);
      if (eqlFn(prevValue.current, nextValuue)) {
        return prevValue.current;
      }
      return (prevValue.current = nextValue);
    }
  };
  return useContextSelector(ctx, patchedSelector);
};

@Morglod
Copy link

Morglod commented Oct 4, 2023

Just found that I implemented pretty similar package, but with a bit more features:

  1. memoization of returned object is supported by default (and custom equal)
  2. no "stale props" problem
  3. has deps for "selector" function

https://github.com/Morglod/use-partial-context

@dai-shi
Copy link
Owner

dai-shi commented Oct 4, 2023

I don't think useMemo is need at all, in fact, it's doing the opposite becuase selector and eqlFn are most likely being updated every render, I believe useRef would best serve the purose (storing prevValue)

import { useContextSelector } from 'use-context-selector';

export const useSelectedContext = (ctx, selector, eqlFn) => {
  const prevValue = useRef(null);
  const patchedSelector = (state) => {
      const nextValue = selector(state);
      if (eqlFn(prevValue.current, nextValuue)) {
        return prevValue.current;
      }
      return (prevValue.current = nextValue);
    }
  };
  return useContextSelector(ctx, patchedSelector);
};

Yeah, we recently implemented useShallow in Zustand with useRef.
pmndrs/zustand#2090

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

No branches or pull requests

10 participants