Bug: useEffect closure not called when dependencies changed

[fabb]

I run into a very weird issue where the dependency of useEffect changes, but its closure is not called. The component renders correctly with the updated value due to a state change of a parent router, just the useEffect closure is not called.

React version: 17.0.0.2 and 18.0.0.rc1

Steps To Reproduce

1. Go to the reproducing CodeSandbox https://codesandbox.io/s/useeffect-bug-oz4k9k?file=/pages/index.tsx
2. Open the CodeSandbox Console
3. Click the "reload without parameters" button and then the "trigger bug" button
4. Observe the logged messages. We see 2 renders with {"status":"updated_value"}, but no useEffect closure call for that value.

Link to code example:

https://codesandbox.io/s/useeffect-bug-oz4k9k?file=/pages/index.tsx

https://github.com/fabb/react-useeffect-bug

Please note that it is a reduced test example from a much bigger project, so don‘t wonder about the weird usage of the ref.

The current behavior

In the reproducing example, the useEffect closure is not called for the updated value. Further changes correctly call the closure though.

The expected behavior

The useEffect closure should be called in the reproducing example. We see that the component renders with the updated value, so the useEffect dependencies definitely changed.

cc @gaearon as discussed on twitter

[satya164]

useRef contains a mutable object, changing a ref wouldn't trigger a re-render or run an effect. this is why refs shouldn't be used in dependency arrays - only state, props or context should be used. you can verify that the official ESLint plugin doesn't add refs to the dependency array.

commenting out your setLoaded triggers the effect because it triggers a re-render. but since you're incorrectly using a mutable value in the dependency array, it's causing unexpected behavior.

so this isn't really a bug in React but expected behavior, it's a bug in your code.

[fabb]

I think you’re misunderstanding the example.

Note that the re-render IS triggered, by a state change of the router caused by the Router.replace. The ref also changes, but we do not rely on the ref to cause the re-render. Afaik the useEffect checks its dependency array on every render of the containing component, thus it should call the closure in this case. It does work in most cases, just not in this one. Since it‘s reliably reproducible, it would be really interesting what the underlying issue is.

The curious thing is that removing one of the setLoaded(true) calls „fixes“ the double re-render with the new value and the useEffect closure is called correctly. My wild guess is that there is optimization logic in react which should prevent unnecessary re-renders when primitive state values are set to the same value again (setLoaded(true) even though loaded is already true), and there is a bug in that optimization logic when refs are involved.

[bearow]

I think it has something to do with ref value updating too slow - When in updateRef() you put setTimeout(() => { setLoaded(true)}, 500) it works as intended.
IMO It is just not a good idea to listen on ref.current in useEffect, even when you'll make it work.

Maybe it's a false trail - I'd say router triggers re-render when the ref value is still not what you'd like it to be.

[ahab8055]

In your current behavior useEffect is run only on First render. When Component is First Render that useEffect runs but on Re-Render useEffect doesn't run. useEffect only runs in two conditions

• On First Render useEffect function runs
• When the state of variable in Dependency array of useEffect Changes

As State of Status doesn't changes after first render so useEffect function doesn't run after first run

[fabb]

But the status DOES change after the first render when clicking the buttons, as can be observed in the console log.

[valen214]

replacing the "Router.replace" line with window.history.pushState( {}, "", window.location.href + "?status=" + newStatus ); like so

can also trigger the the bug and the re-render, but this time the re-render is only triggered once

edit: window.location.origin + window.location.pathname would be better than window.location.href but it doesn't matter in this context

[valen214]

replicated this behavior without useRef and Next
https://codesandbox.io/s/pedantic-bogdan-97rb2m?file=/src/App.tsx

let me re-describe the problem:
the combination of setState({}); setState(<old state>); history.pushState (exactly like the function onChangeStatus) triggers the functional component to be executed again but without the actual render (obviously because the react state machine considers there's no state change).
It is weird though the aforementioned combination invoke the functional component AND not triggering state change.
Some other combinations, like removing one of the lines, putting setState({}) (force state update), either skipped invoking the functional component at all or would correctly introduce useEffect side effect.

edit: think this is related https://reactjs.org/docs/hooks-reference.html#bailing-out-of-a-state-update

[gaearon]

Thanks @valen214 for a compact repro.

There is no bug in React here. The issue is that this code is reading a mutable value during rendering. This makes the behavior unpredictable because the rendering result depends on the moment you read it. When it changes, React won't know to re-render the component. And from React's perspective, if the state has not changed, it's safe to bail out of the update.

Concretely, code like this is wrong:

function MyComponent() {
  let something = window.something // Bad: reading mutable state!
  let something2 = someRef.current // Bad: also reading mutable state!
  useEffect(() => {
    // ...
  }, [something, something2])

If you need to read mutable data during rendering, you should either put it in state and update in useEffect (before 18) or use React.useSyncExternalStore (in 18+).

In the original case (Next.js example), the issue is similarly with storedStatusRef.current — which is reading from a mutable field during render. Render result should only depend on props/state/context. See https://beta.reactjs.org/apis/useref for more info on this.

Hope this helps!

[fabb]

Thanks for the explanation. I was aware that ref value changes don‘t cause rerenders, but not that accessing a ref during rendering could be problematic.

So to get it right - when react bails out of the update, then useEffects that had changed dependencies will also not be executed? Wouldn‘t it be safer if the previous dependency array would be restored in a useEffect when react bails out of an update, so it at least runs the next time the component is rerendered?

It‘s easy for ref values to creep in to the rendering, especially when having nested custom hooks and since there‘s no linting to help prevent this from happening. A bit more forgiving behavior of react would help in that case.

If you need to read mutable data during rendering, you should either put it in state and update in useEffect (before 18) or use React.useSyncExternalStore (in 18+).

Would useMemo be a valid approach, or does it have to be a useState setter?

function MyComponent() {
  const something = useMemo(() => window.something, [window.something])
  const something2 = useMemo(() => window.something, [someRef.current])
  useEffect(() => {
    // ...
  }, [something, something2])

[gaearon]

but not that accessing a ref during rendering could be problematic.

It’s similar to reading window.something during rendering. Either it’s always the same and there’s no need to read it from a mutable field every time. Or it’s sometimes different, which means rendering is not predictable.

when react bails out of the update, then useEffects that had changed dependencies will also not be executed?

Yes.

Wouldn‘t it be safer if the previous dependency array would be restored in a useEffect when react bails out of an update, so it at least runs the next time the component is rerendered?

I’ll ask the team about this. But I wouldn’t count on something so fragile anyway. This should be fixed in the user code.

A bit more forgiving behavior of react would help in that case.

It’s hard to have a “forgiving” behavior here because one you start breaking the rules, the consequences accumulate and become too difficult to reason about. So I actually think overall it’s better when it doesn’t work at all. I hear you re: it being nice for this to be enforced somehow. We haven’t found a great way to do that yet.

Would useMemo be a valid approach, or does it have to be a useState setter?

No, it’s fundamentally the same problem: rendering result, including the emitted effects, should only depend on props/state/context. If you read a mutable value during rendering (in memo or not), your component no longer behaves predictably. Calling it with the same inputs would produce different outputs over time.

[fabb]

Thanks a lot for the detailed explanation!

[fabb]

So this is also not a good idea, right?
vercel/swr#192 (comment)

function useStickyResult (value) {
  const val = useRef()
  if (value !== undefined) val.current = value
  return val.current
}

[ericbiewener]

@gaearon Can you clarify how you'd solve this problem by storing the mutable value in state via useEffect in React 17 as suggested here? To use the problematic example you provided, I took you to mean doing something like this:

function MyComponent() {
  const [something, setSomething] = useState(window.something)

  useEffect(() => {
    setSomething(window.something);
  }, [window.something]);

But this won't work; the entire problem is that the effect function is failing to run! So this doesn't guarantee that the mutable window.something gets synced with state.

Am I misinterpreting your suggested solution?

[fabb]

@ericbiewener as I‘ve learned above, we are not allowed to use useState(window.something), but rather have to use useState(undefined).

[ericbiewener]

@fabb thanks for the quick response, but I'm not sure how useState(undefined) corresponds to window.something. If I ultimately have a useEffect hook that I want to run any time window.something changes, how do I accomplish that?

[fabb]

hook that I want to run any time window.something changes

That‘s only possible if you have registered some listener that listens to changes to that and sets a state in your component so a render is triggered. Depends on your use case and how something is set.

[refractalize]

Hi all - I bumped into this issue recently too, I have to admit it's pretty surprising behaviour. I've made a concise demonstration of the bug here if it helps explain the issue any better: https://codesandbox.io/p/sandbox/trusting-tesla

In short:

const [state, setState] = useState(0);
const dep = useRef(0);
useEffect(fn, [dep.current]);

function bailedUpdate() {
  dep.current++;
  setState(state);
}

function brokenUpdate() {
  setState(state + 1);
}

1. Change the deps, i.e. dep.current++ then set state to the same state, i.e. setState(state). This will trigger a bailed-out render cycle, and useEffect will not trigger. This is expected behaviour AFAICT, according to https://legacy.reactjs.org/docs/hooks-reference.html#bailing-out-of-a-state-update.
2. Finally, do a proper state update setState(state + 1) without changing deps. The deps are still different from the last time useEffect fired, so I'd expect to see it fire but it doesn't. It's as though the bailed-out render updated the deps' previous values while not calling fn. If it's a bailed render, shouldn't it leave the deps' previous values as they were since the last non-bailed-out render?

All of this is covered in the comments above, but I'd like to add that while the above code is pretty contrived (deps pointing to a ref), in practice it's entirely possible that one bit of code doesn't know (or shouldn't know) whether a dep is based on a ref or proper state. Furthermore, it's not always clear that we're calling setState() and setting it to it's current value, causing a bailed-out render cycle. This means that the combination of using useRef and setting state to itself (setState(state)) could lead to missing useEffect fires and very subtle bugs.

I'd like to propose that either:

1. We only update the deps' previous values in the useEffect when it's actually fired, not when useEffect is called but not fired. This would mean that useEffect will fire the next time we have a full render because they compare as different since the last time it was fired, not since it was last queued.
2. We issue a console.warn when the deps have changed but useEffect isn't going to fire because it's a bailed-out render. This would at least point us to ensuring that the deps change only on a proper state change, not due to some external state change, i.e. point us towards staying within the rules.
3. We add some documentation to useEffect that explains why it won't fire in this scenario.