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

useCallback/useEffect support custom comparator #14476

Closed
kpaxqin opened this issue Dec 20, 2018 · 30 comments
Closed

useCallback/useEffect support custom comparator #14476

kpaxqin opened this issue Dec 20, 2018 · 30 comments

Comments

@kpaxqin
Copy link
Contributor

kpaxqin commented Dec 20, 2018

Currently we can pass an array as second argument when using useCallback or useEffect like below:

useCallback(()=> {
  doSth(a, b)
}, [a, b]) // how to do deep equal if a is an object ?

The problem is it only compare array items with ===, it there any way to compare complex object ?

Support custom comparator as third argument looks not bad:

useCallback(()=> {
  doSth(a, b)
  }, 
  [complexObject], 
  (item, previousItem)=> { //custom compare logic, return true || false here }
)
@arianon
Copy link

arianon commented Dec 20, 2018

You could do this instead.

useCallback(() => {
  doSth(a, b)
}, [a, a.prop, a.anotherProp, a.prop.nestedProp /* and so on */, b])

Or you could try using JSON.stringify(a) in the inputs array.

@aweary
Copy link
Contributor

aweary commented Dec 20, 2018

You can use the result of the custom equality check as a value inside the array to get this behavior.

useEffect(
  () => {
    // ...
  },
  [compare(a, b)]
);

@heyimalex
Copy link

@aweary How would that work? They want to use the custom comparator to check compare(a, prevA), not compare(a, b). The previous value isn't accessible in scope.

@aweary
Copy link
Contributor

aweary commented Dec 20, 2018

@heyimalex you can use a ref to store a reference to the previous value. This is mentioned in the Hooks FAQ: How to get the previous props or state?
.

@heyimalex
Copy link

@aweary How would passing [compare(a, prevA)] work? Wouldn't that make it update on transitions from true -> false and from false -> true when you really just want it to update when false? Sorry if I'm missing something obvious!

I was trying to write what the op wanted and here's what I got.

function usePrevious(value) {
    const ref = useRef();
    useEffect(() => {
        ref.current = value;
    });
    return ref.current;
}

function useCallbackCustomEquality(cb, args, equalityFunc) {
    const prevArgs = usePrevious(args);
    const argsAreEqual =
        prevArgs !== undefined &&
        args.length === prevArgs.length &&
        args.every((v, i) => equalityFunc(v, prevArgs[i]));
    const callbackRef = useRef();
    useEffect(() => {
        if (!argsAreEqual) {
            callbackRef.current = cb;
        }
    })
    return argsAreEqual ? callbackRef.current : cb;
}

@aweary
Copy link
Contributor

aweary commented Dec 20, 2018

@kpaxqin yeah it's sort of a half-baked solution 😕your approach is fine I think for useCallback. For useEffect you'd do something similar except you'd add a check for equality inside a useEffect before calling the provided effect callback.

@heyimalex
Copy link

heyimalex commented Dec 20, 2018

You could actually encapsulate the arg memoization in one hook and then use that for all of the other functions.

function useMemoizeArgs(args, equalityCheck) {
  const ref = useRef();
  const prevArgs = ref.current;
  const argsAreEqual =
        prevArgs !== undefined &&
        args.length === prevArgs.length &&
        args.every((v, i) => equalityFunc(v, prevArgs[i]));
  useEffect(() => {
      if (!argsAreEqual) {
        ref.current = args;
      }
  });
  return argsAreEqual ? prevArgs : args;
}

useEffect(() => {
  // ...
}, useMemoizeArgs([a,b], equalityCheck));

@eyston
Copy link

eyston commented Dec 23, 2018

A similar API to React.memo would be great for useEffect and friends when used with an immutable data structure library -- such as clojurescript. Not to over simplify it but it at least appears to be a very accessible change:

export function useEffect(
create: () => mixed,
inputs: Array<mixed> | void | null,
): void {
useEffectImpl(
UpdateEffect | PassiveEffect,
UnmountPassive | MountPassive,
create,
inputs,
);
}
function useEffectImpl(fiberEffectTag, hookEffectTag, create, inputs): void {
currentlyRenderingFiber = resolveCurrentlyRenderingFiber();
workInProgressHook = createWorkInProgressHook();
let nextInputs = inputs !== undefined && inputs !== null ? inputs : [create];
let destroy = null;
if (currentHook !== null) {
const prevEffect = currentHook.memoizedState;
destroy = prevEffect.destroy;
if (areHookInputsEqual(nextInputs, prevEffect.inputs)) {
pushEffect(NoHookEffect, create, destroy, nextInputs);
return;
}
}

@kpaxqin
Copy link
Contributor Author

kpaxqin commented Dec 25, 2018

@heyimalex Thanks for your solution, it works great for me, but still got one risk as a common solution.

For example: re-render every second, use current time as input array, compare time and run effect if minute has changed.

The key is: A equal to B and B equal to C might not means A equal to C for custom comparator

Use another value as compare result could get rid of this

function useMemoizeArgs(args, equalityCheck) {
  const ref = useRef();
  const flag = ref.current ? ref.current.flag : 0;
  const prevArgs = ref.current ? ref.current.args : undefined;
  const argsAreEqual =
        prevArgs !== undefined &&
        args.length === prevArgs.length &&
        args.every((v, i) => equalityFunc(v, prevArgs[i]));
  useEffect(() => {
      ref.current = {
         args,
         flag: !argsAreEqual ? (flag + 1) : flag
      }
  });
  return ref.current.flag
}

useEffect(() => {
  // ...
}, useMemoizeArgs([a,b], equalityCheck));

Still think the best way would be support custom comparator officially for those hooks which need to compare inputs.

@slestang
Copy link

The solution given by @kpaxqin didn't work during my tests because:

  1. ref.current is undefined on first call so return ref.current.flag crash
  2. the way argsAreEqual is compute ends up returning true for the first call, therefore the flag is always incremented at least once

I modify it to:

const useMemoizeArgs = (args, equalityFunc) => {
  const ref = useRef();
  const flag = ref.current ? ref.current.flag : 0;
  const prevArgs = ref.current ? ref.current.args : undefined;
  const argsHaveChanged =
    prevArgs !== undefined &&
    (args.length !== prevArgs.length || args.some((v, i) => equalityFunc(v, prevArgs[i])));

  useEffect(() => {
    ref.current = {
      args,
      flag: argsHaveChanged ? flag + 1 : flag,
    };
  });

  return flag;
};

which works for my use case.

But obviously if hooks supported a custom comparator like React.memo as parameter, things will be a lot better/simpler.

@kalekseev
Copy link

My main use cases where deep equal required is data fetching

const useFetch = (config) => {
  const [data, setData] = useState(null); 
  useEffect(() => {
    axios(config).then(response => setState(response.data)
  }, [config]);  // <-- will fetch on each render
  return [data];
}

// in render
const DocumentList = ({ archived }) => {
  const data = useFetch({url: '/documents/', method: "GET", params: { archived }, timeout: 1000 }) 
  ...
}

I checked top google results for "react use fetch hook", they are either have a bug (refetch on each rerender) or use one of the methods:

So it would be good to have a comparator as a third argument or a section in docs that explains the right way for such use cases.

@gaearon
Copy link
Collaborator

gaearon commented Mar 9, 2019

Right now we have no plans to do this. Custom comparisons can get very costly when spread across components — especially when people attempt to put deep equality checks there. Usually there's a different way to structure the code so that you don't need it.

There are a few common solutions:

Option 1: Hoist it up

If some value is supposed to be always static, there's no need for it to be in the render function at all.
For example:

const useFetch = createFetch({ /* static config */});

function MyComponent() {
  const foo = useFetch();
}

That completely solves the problem. If instantiating it is annoying, you can always have your own useFetch module that just re-exports the instantiated result (or even provides a few variations):

// ./myUseFetch.js
import createFetch from 'some-library';

export const useFetch = createFetch({ /* config 1 */ })
export const useFetchWithAuth = createFetch({ /* config 2 */ })

Option 2: Embrace dynamic values

Another option is the opposite. Embrace that values are always dynamic, and ask the API consumer to memoize them.

function MyComponent() {
  const config = useMemo(() => ({
    foo: 42,
  }, []); // Memoized
  const data = useFetch(url, config);
}

This might seem like it's convoluted. However for low-level Hooks you probably will create higher-level wrappers anyway. So those can be responsible for correct memoization.

// ./myUseFetch.js
import {useContext} from 'react';
import useFetch from 'some-library';
import AuthContext from './auth-context';

export function useFetchWithAuth(url) {
  const authToken = useContext(AuthContext);
  const config = useMemo(() => ({
    foo: 42,
    auth: authToken
  }), [authToken]); // Only changes the config if authToken changes
  return useFetch(url, config);
}

// ./MyComponent.js
function MyComponent() {
  const data = useFethWithAuth('someurl.com'); // No config
}

Then effectively the users don't need to configure anything at the call site — or you can expose limited supported options as specific arguments. That's probably a better API than an arbitrarily deep "options" object anyway.

Option 3: (be careful) JSON.stringify

Many people expressed a desire for deep equality checks. However, deep equality checks are very bad because they have unpredictable performance. They can be fast one day, and then you change the data structure slightly, and they become super slow because the traversal complexity has changed. So we want to explicitly discourage using deep checks — and useEffect API currently does that.

However, there are cases where it stands in the way of ergonomics too much. Such as with code like

const variables = {userId: id};
const data = useQuery(variables);

In this case there is no actual deep comparisons necessary. In this example, we know our data structure is relatively shallow, doesn't have cycles, and is easily serializable (e.g. because we plan to send that data structure over the network as a request). It doesn't have functions or weird objects like Dates.

In those rare cases it's acceptable to pass [JSON.stringify(variables)] as a dependency.

“Wait!” I hear you saying, “Isn’t JSON.stringify() slow?”

On small inputs, like the example above, it is very fast.

It does get slow on larger inputs. But at that point deep equality is also slow enough that you’ll want to rethink your strategy and API. Such as going back to a granular invalidation with useMemo. That gives you the performance — if indeed, you care about it.


There are probably minor other use cases that don't quite fit into these. For those use cases it is acceptable to use refs as an escape hatch. But you should strongly consider to avoid comparing the values manually, and rethinking your API if you feel you need that.

@gaearon gaearon closed this as completed Mar 9, 2019
peternycander added a commit to peternycander/reactjs.org that referenced this issue Mar 17, 2019
Dan listed useMemo as a solution for avoiding re-triggering an effect
facebook/react#14476 (comment)
If this is an accepted use case, the docs should reflect it.
@orestis
Copy link

orestis commented Mar 19, 2019

Just commenting that this is an issue with ClojureScript that provides its own equality semantics, where equality comparison of data structures are based on value and not of reference. So using the Object.is algorithm without an escape hatch is an issue. Similarly for setState.

@jozanza
Copy link

jozanza commented May 21, 2019

@gaearon Option 3 is simple and works well for my case but clashes with react-hooks/exhaustive-deps since the non-serialized object is used in the effect. Is it just not possible with this linting option enabled?

@tboulis
Copy link

tboulis commented Jul 29, 2020

I have built use-custom-compare hooks to solve this. I hope this will help.

Give this man a medal!

@justjake
Copy link

justjake commented Nov 5, 2020

I think it is quite easy to compose a solution to this problem with one or two small hooks, I think. I just started using hooks, though, so perhaps my approach is somehow incorrect

// Returns a number representing the "version" of current.
function useChangeId<T>(next: T, isEqual: (prev: T | undefined, next: T) => boolean) {
  const prev = useRef<T>()
  const id = useRef(0)
  if (!isEqual(prev.current, next)) {
    id.current++
  }
  useEffect(() => { prev.current = next }, [next])
  return id
}

interface FetchConfig { /* ... etc ... */ }

function useFetch(config:  FetchConfig) {
  // Returns a new ID when config does not deep-equal previous config
  const changeId = useChangeId(config, _.isEqual)
  // Returns a new callback when changeId changes.
  return useCallback(() => implementUseFetchHere(config), [changeId])
}

function MyComponent(props: { url: string }) {
  const fetchConfig = { url, ... }
  const fetch = useFetch(fetchConfig)
  return <button onClick={fetch}>Fetch</button>
}

carterworks added a commit to adobe/alloy that referenced this issue Jul 11, 2023
Every time the useSendPageViewEvent() function was being called without
the xdm and data params being specified, the default parameter would
create a new object. This would make the function run again aka it was
not safe to call multiple times because the data and xdm variables
didn't pass the === test.
Since the values are small, there isn't much worry about performance.
See this comment about the subject: facebook/react#14476 (comment)
carterworks added a commit to adobe/alloy that referenced this issue Jul 11, 2023
* Fix (most) eslint errors in sandbox

* Value-compare data and xdm in useEffect dependency arrays

Every time the useSendPageViewEvent() function was being called without
the xdm and data params being specified, the default parameter would
create a new object. This would make the function run again aka it was
not safe to call multiple times because the data and xdm variables
didn't pass the === test.
Since the values are small, there isn't much worry about performance.
See this comment about the subject: facebook/react#14476 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests