React: useCallback() invalidates too often in practice

[gaearon]

This is related to https://github.com/facebook/react/issues/14092, https://github.com/facebook/react/issues/14066, https://github.com/reactjs/rfcs/issues/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 https://github.com/facebook/react/issues/14092#issuecomment-435907249 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 https://github.com/reactjs/rfcs/issues/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.

[muqg]

I recently made a duplicate issue and was asked to check this one. What I proposed there was very similar to @sophiebits' approach, but still looks a bit simpler to me:

function useStatic(cb) {
  const callback = useRef(cb)
  callback.current = cb

  const mem = useRef((...args) => callback.current(...args))
  return mem.current

  // We could, of course still, use the useCallback hook instead of a second reference.
  // return useCallback((...args) => callback.current(...args), [])
  // Although I think that the one above is better since it avoids the need to compare anything at all.
}

This way it is guaranteed to update where the hook is called since it does not directly use any side effect and instead it only updates a reference. It seems to me that it should be callable during the render phase and should not be dicey with concurrent mode (unless references don't meet these two conditions). Wouldn't this approach be a little better or am I missing something?

[gaearon]

@muqg In concurrent mode, last render doesn't necessarily mean "latest committed state". So a low-priority render with new props or state would overwrite a reference used by current event handler.

[strayiker]

If I understand the problem correctly...

What if useCallback will return a special callable object with two different states of an our callback function? The first is a current callback, that will changes on each render. The second is a commited callback, that changes within a commit phase.
By default call to that object will trigger the current value, so it can be used in render phase.
But internally, React will pass the commited value to the dom, which prevents the callback from being modified until the next commit.

function Callback(cb) {
  function callback(...args) {
    return callback.current(...args);
  }

  callback.commited = cb;
  callback.current = cb;
  callback.SPECIAL_MARKER_FOR_REACT = true;

  return callback;
}

function useCallback(cb) {
  const callback = useMemo(() => new Callback(cb), []);

  callback.current = cb;

  useLayoutEffect(() => {
    callback.commited = cb;
  });

  return callback;
}

function Component(counter) {
  const handler = useCallback(() => {
    console.log(counter);
  });

  handler(); // call to handler.current

  // pass handler.commited to the dom
  return <button onClick={handler} />
}

[Volune]

I don't think there is a point in saving the "current", if you want to call it during rendering, just save it in advance out of the hook:

const handler = () => {/* do something*/};
const callback = useCallback(handler);
// I can call the current:
handler();

---

I personally don't see any benefits of the current useCallback implementation over the proposed useEventCallback, will it become the new implementation?
Also, can it warn when the callback is called during render in development mode?

[strayiker]

Concurrent mode can produce two different representations of a component (the first one is that commited to the dom and the second one is that in memory). This representations should behaves accordingly with their props and state.

useEventCallback by @sophiebits mutates ref.current after all dom mutations is completed, so the current (in-memory) component can't use the newest callback until the commit is done.

@muqg proposal mutate the callback on each render, so the commited component will lose the reference to the old callback.

The point of my proposal in the passing a separated callback reference, that will changes in commit phase, to the dom, while the in-memory (not commited) representation of a component can use the latest version of that callback.

const handler = () => {/* do something*/};
const callback = useCallback(handler);

In this case, you wont pass down the handler to other components because it always changes. You will pass the callback, but will face again the concurrent mode problem.

[Voronar]

Hi.
According to @sophiebits useEventCallback implementation why is it
function uses useLayoutEffect and not useEffect for ref updating?
And is it normal due to current limitations use useEventCallback for all internal regular functions with some logic (which wants be memoized for
using in expensive pure components tree or/and has closured variables from outer hook function) inside custom hook?

[jonnyasmar]

How/why is it that variables are dereferenced within useEffect?

Whether or not the effect is called again based on changes to state/reducer/etc (useEffect's second param), shouldn't have any implication on those variable's references within useEffect, right?

This behavior seems unexpected and having to leverage "escape hatches" just feels broken to me.

[Bazzer588]

I have a problem with converting this kind of thing to use functional components and the useCallback hook...

export class TestForm extends React.Component {

    onChangeField = (name,value) => {
        this.setState({ [name]: value });
    };

    render () {
        const state = this.state;
        return (
            <>
                <PickField name="gender"      value={state.gender}      onChangeField={this.onChangeField} />
                <TextField name="firstName"   value={state.firstName}   onChangeField={this.onChangeField} />
                <TextField name="lastName"    value={state.lastName}    onChangeField={this.onChangeField} />
                <DateField name="dateOfBirth" value={state.dateOfBirth} onChangeField={this.onChangeField} />
            </>
        );
    }

The PickField, TextField and DateField components can be implemented with React.PureComponent or React.memo(...). Basically they just display an input field and a label - they have their own onChange handler which calls the onChangeField prop passed in. They only redraw if their specific value changes

_onChangeField_ as above works just fine - but if I try this using a functional component for TestForm and _useCallback_ for onChangeField I just can't get it to 'not' redraw everything on a single field change

[jonnyasmar]

@Bazzer588 Are you using React.memo on your functional component? What do your attempts using hooks/functional look like? Your problem may or may not be related to this issue; it's hard to tell without seeing your code.

[Bazzer588]

Here's the full code - just drop a <TestForm/> or a <HookTestForm/> somewhere on a page

Using a React.Component, only the field being editted does a full render

import React from 'react';
import TextField from "./TextField";

export default class TestForm extends React.Component {

    onChangeField = (name,value) => {
        this.setState({ [name]: value });
    };

    render () {
        const state = this.state || {};
        return (
            <>
                <TextField name="gender"      value={state.gender}      onChangeField={this.onChangeField} />
                <TextField name="firstName"   value={state.firstName}   onChangeField={this.onChangeField} />
                <TextField name="lastName"    value={state.lastName}    onChangeField={this.onChangeField} />
                <TextField name="dateOfBirth" value={state.dateOfBirth} onChangeField={this.onChangeField} />
            </>
        );
    }
}

Using Hooks, all the child elements are re-rendered every time - presumably as onChangeField changes every time the state data changes. Is there some way I can implement onChangeField so it behaves like the above example?

import React, {useState, useCallback} from 'react';
import TextField from "./TextField";

export default React.memo( () => {

    const [data, changeData] = useState({});

    const onChangeField = useCallback((name,value) => {
        changeData({ ...data, [name]: value });
    }, [data] );

    return (
        <>
            <TextField name="gender"      value={data.gender}      onChangeField={onChangeField} />
            <TextField name="firstName"   value={data.firstName}   onChangeField={onChangeField} />
            <TextField name="lastName"    value={data.lastName}    onChangeField={onChangeField} />
            <TextField name="dateOfBirth" value={data.dateOfBirth} onChangeField={onChangeField} />
        </>
    );
});

This is my <TextField> component - you can see when it does a full render from the console or with the React dev tools

import React from 'react';

export default React.memo( ({ name, value, onChangeField }) => {

    console.log('RENDER TEXT FIELD',name,value);

    return (
        <div className="form-field">
            <label htmlFor={name}>{name}</label>
            <input
                type="text"
                onChange={ (ev) => onChangeField( name, ev.target.value ) }
                value={value || ''}
            />
        </div>
    );
});

[muqg]

@Bazzer588 I think its due to the object value kept in state under the variable name of data. I don't know why but objects in state always invalidate memoization and thus your callback onChangeField is a new on on each render thus breaking the memoization of the components you're passing it to.

I've had a similar issue like you and noticed this as being its cause. I have no idea why the object in state does not keep its reference when it has not been explicitly set to a new object.

[Bazzer588]

Yes my problem is that the in first example (using React.Component) I can create a _onChangeField_ callback which is bound to _this_, and never changes during renders

Using the new hook methods I can't seem to replicate the way the existing functionality works,

On the project I'm working on we often do this to have a hierachy of components and state:

    onChangeField = (fieldName,fieldValue) => {
        const newValue = { ...this.props.value, [fieldName]: fieldValue };
        this.props.onChangeField( this.props.name, newValue );
    };

So it passes props down the tree (ie value {})
And uses the callback to send new values up the tree

[dantman]

Using the new hook methods I can't seem to replicate the way the existing functionality works,

Use useReducer.

[jonnyasmar]

@Bazzer588 The data var your passing to the second parameter of useCallback is going to invalidate every time. useCallback doesn't do a deep comparison. You need to pass in a flat array for it to properly memoize.

[Bazzer588]

Yes I've tried similar with _useReducer_ and passing _dispatch_ down to the child components - this does seem to work as you get the same _dispatch_ method so it doesn't force a re-render

If there's no way to make callback functions work as they did before I guess that's the way we'll have to go

[sebmarkbage]

@Bazzer588 The general recommendation is to useReducer.

That said, your particular example can be solved like this but it is pretty unusual to not depend on any props so this doesn't always work:

    const onChangeField = useCallback((name, value) => {
        changeData(oldData => ({ ...oldData, [name]: value }));
    }, []);

[Bazzer588]

Thanks @sebmarkbage - didn't realise you could pass a function to changeData
(As in const [data, changeData] = useState({}); )
However now I'm worried - in what case would this _doesn't always work_ ?

[doasync]

@gaearon, is this issue going to be solved before hooks release?

[gaearon]

Workarounds above (especially relying more on useReducer) seem sufficient for most cases. There are cases when they’re not, but we’ll likely revisit this again in a few months.

[Bazzer588]

It's very, very different behaviour

With a ES6 class I can pass _onChange={this.handleChange}_ to the child component and it does not redraw every time (because it sends the same _handleChange_ method every time)

It's frustrating that _dispatch_ works independent of state (I mean the _dispatch_ method does not change even if the underlying state does) but useCallback does not (if your callback needs to do something with state that is)

I see a lot of code where the whole form redraws when the use types into a field

A lot of devs don't understand this until users complain 'it's slow'

[carlosagsmendes]

@Bazzer588 The general recommendation is to useReducer.

That said, your particular example can be solved like this but it is pretty unusual to not depend on any props so this doesn't always work:

    const onChangeField = useCallback((name, value) => {
        changeData(oldData => ({ ...oldData, [name]: value }));
    }, []);

@sebmarkbage I tried the approach above of using the oldData argument, but it didn't help me to avoid re-renders of the children components (that's why I was using useCallback)

So I tried using useReducer and use dispatch inside the useCallback but all the children are still re-rendering.

Moreover, I'm passing the state to a callback after changing it, and it is always the previous state (off by one).

Can someone take a look and let me know what the recommended approach is?

Thanks in advance!

[Volune]

Workarounds above (especially relying more on useReducer) seem sufficient for most cases. There are cases when they’re not, but we’ll likely revisit this again in a few months.

@gaearon then do you plan to release useCallback with the first release of the hooks? After all it is confusing (current issue) and it is not a primitive hook (is equivalent to useMemo(() => fn, inputs))

---

@carlosagsmendes you can probably use the workaround useEventCallback from above. You can see your example with it here

I put the useEventCallback (with couple changes) here again:

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

[HaNdTriX]

I worked around this issue, by creating the following hook:

Code:

import { useRef, useEffect } from 'react'

/**
 * This hook reduces the amount of rebindings.
 * Use it when your props or state change a lot 
 * or when you have nested handlers.
 *
 * @param {DomRef} targetRef The reference to the dom node
 * @param {String} eventName The eventName you want to bind
 * @param {Function} handler The actual event handler
 */
function useCachedRefHandler (targetRef, eventName, handler) {
  const savedHandler = useRef()

  useEffect(() => {
    savedHandler.current = handler
  }, [handler])

  useEffect(() => {
    const internalHandler = (...args) => {
      if (savedHandler.current) {
        savedHandler.current(...args)
      }
    }
    targetRef.current.addEventListener(eventName, internalHandler)
    return () => targetRef.current.removeEventListener(eventName, internalHandler)
  }, [])
}

export default useCachedRefHandler

This code allows me to omit useCallback completely and therefore use state and props without diffing.

Usage Example:

import React, { useRef } from 'react'
import useCachedRefHandler from './hooks/useCachedRefHandler'

function Webview({ onSomeingA, onSomethingB, onSomethingC }) {
  const ref = useRef()

  useCachedRefHandler(ref, 'ipc-message', (event) => {
    switch (event.channel) {
      case 'someingA':
        onSomeingA (event.data)
        return
      case 'someingB':
        onSomeingB (event.data)
        return
      case 'someingC':
        onSomeingB (event.data)
        return
    }
  })

  return (
    <webview ref={ref} />
  )
}

export default Webview

[eps1lon]

Had a use case for @sophiebits proposed useEventCallback (https://github.com/facebook/react/issues/14099#issuecomment-440013892). The problem with useLayoutEffect is that it issues warnings when server side rendering. You can trick React by switching to useEffect based on the environment (typeof window) but this won't work if the SSR API is called in the browser (with-apollo).

Wouldn't useImperativeHandle work as well?

Edit: __WARNING__ This means that the callback __cannot__ be called safely in the effect cleanup phase. When unmounting the ref will be nulled before the cleanup phase.

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

[sompylasar]

I found myself doing this often:

function Foo({ someProp, otherProp }) {
  const somePropRef = useRef(someProp);
  useLayoutEffect(() => {  // or useEffect
    somePropRef.current = someProp;
  }, [someProp]);

  const otherPropRef = useRef(otherProp);
  useLayoutEffect(() => {  // or useEffect
    otherPropRef.current = otherProp;
  }, [otherProp]);

  const onSomething = useCallback(() => {
    handleSomething(somePropRef.current, otherPropRef.current);
  }, []);

  return <Bar onSomething={onSomething} />;

  // or...
  //
  // // This ref value never changes, so can use .current in render.
  // // WARNING: Safe to close over refs and outer scope consts only.
  // const onSomethingRef = useRef(() => {
  //   handleSomething(somePropRef.current, otherPropRef.current);
  // });
  //
  // return <Bar onSomething={onSomethingRef.current} />;
}

Does that have any drawbacks besides unnecessary effects?

[arnotes]

I found myself doing this often:

function Foo({ someProp, otherProp }) {
  const somePropRef = useRef(someProp);
  useLayoutEffect(() => {  // or useEffect
    somePropRef.current = someProp;
  }, [someProp]);

  const otherPropRef = useRef(otherProp);
  useLayoutEffect(() => {  // or useEffect
    otherPropRef.current = otherProp;
  }, [otherProp]);

  const onSomething = useCallback(() => {
    handleSomething(somePropRef.current, otherPropRef.current);
  }, []);

  return <Bar onSomething={onSomething} />;

  // or...
  //
  // // This ref value never changes, so can use .current in render.
  // // WARNING: Safe to close over refs and outer scope consts only.
  // const onSomethingRef = useRef(() => {
  //   handleSomething(somePropRef.current, otherPropRef.current);
  // });
  //
  // return <Bar onSomething={onSomethingRef.current} />;
}

Does that have any drawbacks besides unnecessary effects?

@sompylasar I wanna know as well. we are doing the same thing using this hook

useController<T extends IPropsAndState>(propsAndState:T):T{
  const controllerRef = useRef({});
  Object.assign(controllerRef.current, propsAndState);
  return controllerRef.current;
}
....

....

we badly need it since we have a websocket rjxs subscription and we need an updated context for the callback functions inside filter pipes and the actual subscription.

i cant use a useeffect that contains dependencies in my case because i dont want to constantly disconnect/reconnect to our socket server

[nikparo]

@arnotes Looks like you are updating your ref during the render phase, i.e. before it has been commited, rather than in a useEffect call. To quote @gaearon :

In concurrent mode, last render doesn't necessarily mean "latest committed state". So a low-priority render with new props or state would overwrite a reference used by current event handler.

Afaik the drawback with updating the ref in a useEffect is mainly that you then can't use it during the render.

[bvaughn]

Does that have any drawbacks besides unnecessary effects?

Late to the party here, but after scanning the above thread- there is another potential problem in the useEventCallback example and other example uses of useLayoutEffect above:

function Example({ callback }) {
  const stableCallback = useEventCallback(callback);
  return <Child callback={stableCallback} />;
}

function Child({ callback }) {
  useLayoutEffect(() => {
    // Callback has not yet been updated!
    // This is because child effects are run before parent effects,
    // So the callback ref has not yet been updated (and still points to the old value).
    callback();
  }, [callback]);
}

[yelouafi]

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

Aren't class components also exposed to this? Seems React does the same thing to Class Components (mutating props in render phase)

https://github.com/facebook/react/blob/4c5698400f04bbc6d0b4bd766b0993d0bcb37609/packages/react-reconciler/src/ReactFiberClassComponent.js#L1042-L1045

also seems the committed and work-in-progress fibers share the same class instance

https://github.com/facebook/react/blob/4c5698400f04bbc6d0b4bd766b0993d0bcb37609/packages/react-reconciler/src/ReactFiber.js#L413

If React assigns pending props to instance.props in the render phase but the render work is interrupted for some reason, this.props will reflect the props of the WIP (outside of render phase) since both share the same instance. If an event handler fires in between and accesses this.props it'll see the WIP props while it should rather seethe committed props.

I assume the solution is to switch this.props between phases, maybe this is already done but I couldn't trace it in the sources so just wanted to make sure this is handled.

[Hypnosphi]

@vzaidman

The solution by @sophiebits does the same without a need for stating any deps at all. The callback from ref.current always has a fresh closure. So the usage looks like this

const handleClick= useEventCallback(() => {
  // do anything with props, state, or any other local variables
})

[vzaidman]

missed it altogether and deleted my comment. thanks

[Hypnosphi]

@Volune

I personally don't see any benefits of the current useCallback implementation over the proposed useEventCallback

Current useCallback is quite helpful with render props pattern. One example is List from react-virtualized. It's a PureComponent, so you can pass a memoized rowRenderer function to prevent rerenders. But if your rowRenderer doesn't invalidate when its deps change (e.g. if it's an instance property on a class component), you have to pass all those deps as List props to trigger rerenders when they're actually needed.

Example on codesandbox: https://codesandbox.io/embed/staging-snow-sgh7v
Note how I have to pass an extra value property to List to fix the class example while useCallback works as is because it invalidates whenever it would produce a different output.

@muqg @strayiker BTW this is why there's no much sense in trying to make useEventCallback work in the render phase: If a child uses your callback to render something, you most likely _want_ it to invalidate as often as its deps change. Also, this is why useEventCallback is a really good name. Basically, it says "only use me for event handlers".

[artem-malko]

@gaearon @sebmarkbage @lastw

Lets imagen, we have react-redux in our App. So, in any component, where we are going to dispatch something we will code something like this:

const Comp = React.memo((props) => {
  // This is from react-redux
  const dispatch = useDispatch();
  const handler = useCallback(() => {
    // actionCreator is created in our project and it is static function
    dispatch(actionCreator(props));
  }, [props]);

  return <Button onClick={handler}></Button>
})

Button component will receive new handler every time, than props are changed. As I understand, this is the main theme of the issue)

What if we will create our own custom hook useHandler, which will look like:

// actionCreator won't be changed, just static function in our case
const useHandler = (actionCreator, params) => {
  // This is from react-redux. Dispatch won't be changed.
  const dispatch = useDispatch();
  const paramsRef = useRef(params);
  // This is useless for the first time, but we need it later
  // This code can be moved or we can add a condition that paramsRef.current !== params.
  paramsRef.current = params;
  // callback will be created only on the first call of useHandler, cause of []
  // But it will dispatch action with new params on next calls
  const callback = useCallback(() => {
    dispatch(actionCreator(paramsRef.current));
  }, []);

  return callback;
}

So, if we have static actionCreator, static dispatch from useDispatch — is it safe to use that approach in concurrent mode?

By the way, how can we understand, what is safe to use in concurrent mode, and what will break our app?

[bvaughn]

This line in your above example could cause problems:

paramsRef.current = params;

Since you're mutating the ref during render- and renders can be async- you can end up mutating it prematurely (so that a previously-rendered thing calls the callback and gets a not-yet-rendered params value.

To avoid this, you would want to update the ref in a layout effect (useLayoutEffect) although this has its own drawbacks (see this comment).

Edit for clarity.

[artem-malko]

@bvaughn but in that case it is possible to have a bug like in your example here https://github.com/facebook/react/issues/14099#issuecomment-534673602
useEffect will be preferable. What do you think?

[bvaughn]

Yup! Still has potential for bugs. Mutating during during render seems worse to me though.

[artem-malko]

As I understand, we can use safely a value from useRef in any hook, but we can not change it safely. Only React itself can do it. For example, if we use useRef to have a ref on dom-node — it is ok, cause React change current value.

It's not about current issue only. It's a general conclusion about useRef and concurrent mode.

[bvaughn]

That doesn't sound right. Our docs list examples of using the ref for non-React values. The key is when to update the .current property. React updates this property during the "commit phase" (when effects are run) and we suggest you do the same¹ to avoid problems like the one I mentioned above.

¹ The one exception to this is using a ref to lazily initialize a value. This is only safe because it's idempotent.

[artem-malko]

@bvaughn
Ok, thanks. Now it's much more clear for me) useEffect/useLayoutEffect is not a silver bullet, but a compromise for useHandler in case, when fn for callback is just static function and only params are changing.

So, will wait for new hook, which is based on useReducer or something like this.

[igodorogea]

@sophiebits Why is the comma expression needed here?

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

[ljharb]

It ensures that the receiver (this value) of the function isn’t ref.

[ypresto]

@eps1lon
https://github.com/facebook/react/issues/14099#issuecomment-499781277

The useImperativeHandle() solution looks good, but it turns out that ref.current will not be set when the callback is called in useLayoutEffect() placed in a child component.
(Original "callback is outdated" issue is here: https://github.com/facebook/react/issues/14099#issuecomment-534673602 )

https://codesandbox.io/s/react-useeventcallback-test-eirgj
This PoC code shows that ref.current is always not set at callback call.

App: rendering text="abc"
Child: calling layout callback
Callback called: ab
Child: calling imperative callback
!!! imperative callback: ref.current == null
layout callback: ref.current = fn

[ypresto]

The useImperativeHandle() solution still does not call outdated func at least. So it can be used where you are confident that the callback will not be called from useLayoutEffect().

[jedwards1211]

Tbh this is making me super skeptical of concurrent mode, given that function memoization is the number one tedious thing I deal with daily in React since hooks were introduced (everything else about hooks being fantastic) and concurrent mode wouldn't benefit me personally right now AFAIK, yet sounds quite dangerous to use.

[mariusGundersen]

I came across this problem when I tried to call one callback from another one, and observed strange effects. A very simplified example:

const MyComponent = props => {
  const [value, setValue] = useState();
  const [copy, setCopy] = useState();

  const createCopy = useCallback(() => {
    setCopy(value);
  }, [value]);

  const callback = useCallback(() => {
    createCopy();
  }, []);

  return (
    <div>
      <input onChange={e => setValue(e.target.value)} />
      <button onClick={callback}>Copy</button>
      <span>{copy}</span>
    </div>
  );
};

This is obviously a simplified and therefore contrived example. It does not behave how I would expect it to.

According to the documentation it is safe to use setCopy from within a callback (or effect) without adding it to the dependency list. But it appears it's not safe to call another callback created with useCallback without adding it to the dependency list. There is probably a good reason for it, but it seems weird to me and clearly several other people in this thread.

So in the above example calling callback() will not update copy, since it calls an old version of createCopy which holds an old version of value.

[Hypnosphi]

@mariusGundersen setCopy is guaranteed to remain the same through component's lifetime, while createCopy changes each time when value does

[mariusGundersen]

Yes, that's clearly how it behaves. But it also means that callback needs to change too. It would be useful and consistent if useCallback behaved like useState.

[davidnussio]

@mariusGundersen If you want callback change too, it need setCopy as its dependency.
In this way it is guarantee that no side effects happened

[liorJuice]

I found that you can use useReducer to mitigate this effectively. the bit that was hard to swallow is when i needed side effects to happen in the callback.

Side effects cant happen in the reducer as it is being called with past actions _a lot_, on re renders, for some reason. so the reducer must be pure (state, action) => resultState, with no side effects.

The solution was to use useEffect that listen to changes in the reduced state (resultState), to initiate side effects, which suddenly seemed obvious. 😅

for example, this code:

  const [counter, setCounter] = useState(0);

  const handleClick = useCallback(() => {
    console.log(`child clicked ${counter + 1} times`);
    setCounter((c) => c + 1);
  }, [counter]);

which will create a very problematic callback, could be perfectly replaced with:

  const [counter, handleClick] = useReducer((c) => c + 1, 0);

  useEffect(() => {
    counter && console.log(`child clicked ${counter} times`);
  }, [counter]);

which will have the same effect but with handler that will persist through the entire lifetime of the component.

i think this is the most complete solution to the useCallback problem.
@gaearon can you confirm?

[jedwards1211]

@liorJuice I wonder if in some cases people would need side effects to happen synchronously, right away when something is clicked though?

[Hypnosphi]

@liorJuice how would you use props in side effects with this approach?

[liorJuice]

@Hypnosphi like other effect dependencies. remember that if if you only use them for calculations - you can safely move them to the reducer.

[Hypnosphi]

@liorJuice

like other effect dependencies

then the effect will trigger whenever the prop changes which is probably not what you want.

I'm talking about cases like that:

const handleClick = useCallback(() => {
  console.log(props.name);
}, [props.name]);

How would you apply your approach to this case?

[jedwards1211]

You wouldn't need to pass props used in side effects as dependencies -- the new values for those props would be part of the new closure when the effect fires because the counter dependency changed.

  useEffect(() => {
    counter && console.log(`${props.name}: child clicked ${counter} times`);
  }, [counter]);

Though I assume it may be possible for the props to change between the click that incremented the counter and the render that triggers the effect?

[liorJuice]

@Hypnosphi the problem you describe is a general useEffect problem (how to write effects with dependencies that don't need to trigger the effect),
it is not specific to the the useCallback solution i described.
there are number of approaches you can take to solve prop dependencies in effects.

i suggest taking a look here:
https://overreacted.io/a-complete-guide-to-useeffect/#why-usereducer-is-the-cheat-mode-of-hooks

there is also a custom hook named "usePrevious", you can use it to check if a specific dependency changed on useEffect. im not sure how much of a best practice it is.

here is one solution:

  const [{logReport}, handleClick] = useReducer((state) => ({
    counter: state.counter + 1,
    logReport: `${props.name}: child clicked ${state.counter + 1} times`
  }), {counter: 0, logReport: ''});

  useEffect(() => {
    logReport && console.log(logReport);
  }, [logReport]);

you can use props inside reducers if you put the reducer function inside the component (which you shouldn't do by default, but you can).
note that logReport does not change when props.name changes, but only when the user clicks.

[nikparo]

I was inspired by @liorJuice's thoughts on useReducer -> useEffect above to create a custom hook as an experiment, use-effect-callbacks. It transforms an object of callbacks into stable methods that can then be passed down. It works by caching the callbacks through a useReducer and executing them in the next useEffect. Could be used as such:

import { useEffectCallbacks } from 'use-effect-callbacks';

// ...

// Within parent component
const [state, setState] = useState({ foo: 'bar' });
const { getState } = useEffectCallbacks({
  getState: () => state,
});

// ...

// Within memoized child component
const handleClick = async () => {
  setState({ foo: 'baz' });
  const latestState = await getState();
  console.log(latestState); // -> { foo: 'baz' }
};

Pros:

• It should be concurrent safe.
• The methods are stable as long as the callback names are stable.
• It is quite straightforward and simple to use.
• You don't need to manually keep track of a lot of different properties in your state to trigger various effects. (Compared to the more manual approach proposed by @liorJuice.)

Cons:

• It's a bit wasteful.
  
  
  
  • Calling a method will trigger the owner component to re-render. (Not a problem if you expect it to re-render anyway, e.g. due to a setState call)
  
  
  • If a method is called in a useEffect, I believe it will still trigger a full render-cycle and only execute in the next useEffect.
  
  
• The methods are not suitable as event listeners since they execute later, and any react event would have been recycled. (They can however very well be called from event listeners)
• While the methods can "safely" be called during renders, there's not much point in doing so since they return promises.

Conclusion:

To be perfectly honest I'm still not quite sure what to think of this approach. It seems handy for occasionally getting the next state, but most other use-cases I can think of seem like a bit of a stretch.

[ypresto]

FYI: My snippet of useImperativeHandle solution https://github.com/facebook/react/issues/14099#issuecomment-569044797 is like this (it is TypeScript, remove type annotations for JS):

// Better useCallback() which always returns the same (wrapped) function reference and does not require deps array.
// Use this when the callback requires to be same ref across rendering (for performance) but parent could pass a callback without useCallback().
// useEventCallback(): https://github.com/facebook/react/issues/14099#issuecomment-499781277
// WARNING: Returned callback should not be called from useLayoutEffect(). https://github.com/facebook/react/issues/14099#issuecomment-569044797
export function useStableCallback<T extends (...args: any[]) => any>(fn: T): T {
  const ref = useRef<T>()
  useImperativeHandle(ref, () => fn) // Assign fn to ref.current (currentFunc) in async-safe way

  return useRef(((...args: any[]) => {
    const currentFunc = ref.current
    if (!currentFunc) {
      throw new Error('Callback retrieved from useStableCallback() cannot be called from useLayoutEffect().')
    }
    return currentFunc(...args)
  }) as T).current
}