Potential bug in "official" useInterval example

Question

useInterval

useInterval from this blog post by Dan Abramov (2019):

function useInterval(callback, delay) {
  const savedCallback = useRef();

  // Remember the latest callback.
  useEffect(() => {
    savedCallback.current = callback;
  }, [callback]);

  // Set up the interval.
  useEffect(() => {
    function tick() {
      savedCallback.current();
    }
    if (delay !== null) {
      let id = setInterval(tick, delay);
      return () => clearInterval(id);
    }
  }, [delay]);
}

A Potential Bug

The interval callback may be invoked between the commit phase and the useEffect invocation, causing the old (and hence not up to date) callback to be called. In other words, this may be the execution order:

1. Render phase - a new value for callback.
2. Commit phase - state committed to DOM.
3. useLayoutEffect
4. Interval callback - using savedCallback.current(), which is different than callback.
5. useEffect - savedCallback.current = callback;

React's Life Cycle

To further illustrate this, here's a diagram showing React's Life Cycle with hooks:

Dashed lines mean async flow (event loop released) and you can have an interval callback invocation at these points.

Note however, that the dashed line between Render and React updates DOM (commit phase) is most likely a mistake. As this codesandbox demonstrates, you can only have an interval callback invoked after useLayoutEffect or useEffect (but not after the render phase).

So you can set the callback in 3 places:

• Render - Incorrect because state changes have not yet been committed to the DOM.
• useLayoutEffect - correct because state changes have been committed to the DOM.
• useEffect - incorrect because the old interval callback may fire before that (after layout effects) .

Demo

This bug is demonstrated in this codesandebox. To reproduce:

• Move the mouse over the grey div - this will lead to a new render with a new callback reference.
• Typically you'll see the error thrown in less than 2000 mouse moves.
• The interval is set to 50ms, so you need a bit of luck for it to fire right between the render and effect phases.

Use Case

The demo shows that the current callback value may differ from that in useEffect alright, but the real question is which one of them is the "correct" one?

Consider this code:

const [size, setSize] = React.useState();

const onInterval = () => {
  console.log(size)
}

useInterval(onInterval, 100);

If onInterval is invoked after the commit phase but before useEffect, it will print the wrong value.

Answer 1

This does not look like a bug to me, although I understand the discussion.

The answer above that suggests updating the ref during render would be a side effect, which should be avoided because it will cause problems.

The demo shows that the current callback value may differ from that in useEffect alright, but the real question is which one of them is the "correct" one?

I believe the "correct" one is the one that has been committed. For one reason, committed effects are the only ones that are guaranteed to have cleanup phase later. (The interval in this question doesn't need a cleanup effect, but other things might.)

Another more compelling reason in this case, perhaps, is that React may pre-render things (either at lower priority, or because they're "offscreen" and not yet visible, or in the future b'c of animation APIs). Pre-rendering work like this should never modify a ref, because the modification would be arbitrary. (Consider a future animation API that pre-renders multiple possible future visual states to make transitions faster in response to user interaction. You wouldn't want the one that happened to render last to just mutate a ref that's used by your currently visible/committed view.)

---

Edit 1 This discussion mostly seems to be pointing out that when JavaScript isn't synchronous (blocking), when it yields between rendering, there's a chance for other things to happen in between (like a timer/interval that was previously scheduled). That's true, but I don't think it's a bug if this happens during render (before an update is "committed").

If the main concern is that the callback might execute after the UI has been committed and mismatch what's on the screen, then you might want to consider useLayoutEffect instead. This effect type is called during the commit phase, after React has modified the DOM but before React yields back to the browser (aka so no intervals or timers can run in between).

---

Edit 2 I believe the reason Dan originally suggested using a ref and an effect for this (rather than just an effect) was because updates to the callback wouldn't reset the interval. (If you called clearInterval and setInterval each time the callback changed, the overall timing would be interrupted.)

Answer 2

To attempt to answer your final question strictly:

I can't see any logical harm updating the callback in render() as opposed to useEffect(). useEffect() is never called anything other than after render(), and whatever it is called with will be what the last render was called with, so the only difference logically is that the callback may be more out-of-date by the time the useEffect() is called.

This may be exacerbated by the coming concurrent mode, if there may be multiple calls to render() before a call to useEffect(), but I'm not even sure it works like that.

However: I would say there is a maintenance cost to doing it this way: it implies that it is ok to cause side effects in render(). In general that is not a good idea, and all necessary side effects should really be done in useEffect(), because, as the docs say:

the render method itself shouldn’t cause side effects ... we typically want to perform our effects after React has updated the DOM

So I would recommend putting any side effect inside a useEffect() and having that as a coding standard, even if in certain situations it is OK. And particularly in a blog post by a react core dev that is going to be copied and pasted by "guide" many people it is important to set the right example ;-P

Alternative solution

As for how you can fix your problem, I will just copy and paste my suggested implementation of setInterval() from this answer, which should remove the ambiguity by calling the callback in a separate useEffect(), at which point all state should be consistent and you don't have to worry about which is "correct". Dropping it into your sandbox seemed to solve the problem.

function useTicker(delay) {
  const [ticker, setTicker] = useState(0);
  useEffect(() => {
    const timer = setInterval(() => setTicker(t => t + 1), delay);
    return () => clearInterval(timer);
  }, [delay]);

  return ticker;
}

function useInterval(cbk, delay) {
  const ticker = useTicker(delay);
  const cbkRef = useRef();
  // always want the up to date callback from the caller
  useEffect(() => {
    cbkRef.current = cbk;
  }, [cbk]);

  // call the callback whenever the timer pops / the ticker increases.
  // This deliberately does not pass `cbk` in the dependencies as
  // otherwise the handler would be called on each render as well as
  // on the timer pop
  useEffect(() => cbkRef.current(), [ticker]);
}

Answer 3

Here is a modification of your example which shows that both/neither approaches are correct: https://codesandbox.io/s/useintervalbug-neither-are-correct-zu2zt?file=/src/App.js

The use of refs is not what you would do in reality but it was necessary to easily detect and report the problem. They do not materially affect the behaviour.

In this example the parent component has created the new "correct" callback, finished rendering and wants that new callback to be used by the child and the timer.

Ultimately there is a race condition between when the "correct" callback ultimately gets passed to useInterval and when the browser decides to call the callback. I do not think it is possible to avoid this.

It makes no difference if you memoize the callback unless of course it has no dependencies and never changes.