useCallback() invalidates too often in practice

[gaearon]

This is related to #14092, #14066, reactjs/rfcs#83, and some other issues.

The problem is that we often want to avoid invalidating a callback (e.g. to preserve shallow equality below or to avoid re-subscriptions in the effects). But if it depends on props or state, it's likely it'll invalidate too often. See #14092 (comment) for current workarounds.

useReducer doesn't suffer from this because the reducer is evaluated directly in the render phase. @sebmarkbage had an idea about giving useCallback similar semantics but it'll likely require complex implementation work. Seems like we'd have to do something like this though.

I'm filing this just to acknowledge the issue exists, and to track further work on this.

[edkalina]

@gaearon thank you for your answer in reactjs/rfcs#83. I've look at sources of useReducer. But I can't understand how it is related to useCallback. What issues has "mutation of ref during rendering"? Can you explain me in brief?

[gaearon]

I've look at sources of useReducer. But I can't understand how it is related to useCallback

useCallback lets you memoize the callback to avoid a different function being passed down every time. But you have to specify everything it depends on in the second array argument. If it's something from props or state, your callback might get invalidated too often.

useReducer doesn't suffer from this issue. The dispatch function it gives you will stay the same between re-renders even if the reducer itself closes over props and state. This works because the reducer runs during the next render (and thus has natural ability to read props and state). It would be nice if useCallback could also do something like this but it's not clear how.

What issues has "mutation of ref during rendering"? Can you explain me in brief?

In concurrent mode (not yet released), it would "remember" the last rendered version, which isn't great if we render different work-in-progress priorities. So it's not "async safe".

[strayiker]

Would be nice if second argument of useCallback was injected as dependencies to callback function.

  function useCallback(cb, deps) => {
    lastDeps = deps; // save current deps and cb deep in somewhere
    lastCb = cb;

    if (!cached) {
      cached = (...args) => lastCb(...lastDeps)(...args); // memoize that forevere
    }

    return cached; // never invalidates
  }

  const myCallback = useCallback(
    (state, props) => (a, b) => a + b + state + props,
    [state, props]
  );

  myCallback(1, 2)

[sokra]

const useCallback = (fn, args) => {
  const callback = useMemo(() => {
    if (__DEV__) {
      if (fn.length !== args.length) warning(...);
    }
    const callback = () => fn(...callback.args);
    return callback;
  });
  useEffect(() => callback.args = args, [args]);
  return callback;
}

Drawbacks:

It's easy to forget the arguments list, which would result in hard to find bugs. In dev mode it would make sense to check fn.length for the correct length.

It's still possible to forget arguments in the dependencies array, but this applies to other hooks too.

[gaearon]

Yes, that's the approach from reactjs/rfcs#83 and https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback. We don't want it to be default because it's easier to introduce bugs in concurrent mode this way.

[sophiebits]

@sokra An alternate would be:

function useEventCallback(fn) {
  let ref = useRef();
  useLayoutEffect(() => {
    ref.current = fn;
  });
  return useCallback(() => (0, ref.current)(), []);
}

This doesn't require the args like yours has. But again, you can't call this in the render phase and the use of mutation is dicey for concurrent.

[sokra]

@sophiebits That's clever and would have none of the problems with args, etc. It even doesn't require a dependencies list.

One nitpick: return useCallback((...args) => (0, ref.current)(...args), []); to pass along i. e. event argument.

[strayiker]

@sokra With this you will not be able to access to state and props updates inside a callback.

const [state, setState] = useState(0);

const handleClick = useCallback((event) => {
  console.log(state); // always 0
  setState(s => s + 1);
});

return <button onClick={handleClick} />

So dependencies are required.

function useCallback(fn, deps) {
  const fnRef = useRef(fn);
  const depsRef = useRef(deps);

  useLayoutEffect(() => {
    fnRef.current = fn;
    depsRef.current = deps;
  });

  return useCallback((...args) => (0, ref.current)(...depsRef.current)(...args), []);
}

cons handleClick = useCallback(
  (state) => event => console.log(state), // up-to-date
  [state]
);

[sokra]

@sokra With this you will not be able to access to state and props updates inside a callback.

I think with @sophiebits' approach this will work. The latest function is always copied into the ref and only a trampoline function is returned. This will make sure that the latest function is called, which has the latest state in context.