facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.16k stars 46.31k forks source link

Bug: effect runs with stale state values outside of Concurrent React #30291

Open denis-sokolov opened 2 months ago

denis-sokolov commented 2 months ago
const [done, setDone] = useState(false);
useEffect(() => {
  if (done) return;
  setDone(true);
  console.log("šŸ¤” This runs more than once.");
});

I have held an assumption that a React effect will only run once the state change queue has been processed. In other words, a new run of a useEffect would only be kicked off once the state values are settled. This does not seem to be the case in the examples below.

I understand that Concurrent React might throw a wrench in this model, but the examples below run on React 18.3, and no concurrent features are used. In particular, I canā€™t identify any low or high priority changes in the examples.

I understand that state change batching exists. But I expected for the batching to finish before running effects.

I fully admit, that it is likely my mental model of React here is wrong. However, if the effects are not guaranteed to run before the state changes are settled, then does that mean that we can not reason about the state of state values in effects at all and should be ready for anything? It seems too weak of a guarantee for common use, which is why I am hesitant to accept it as the simplest explanation.

Regardless, any response will be highly appreciated!

React version: 18.3.1

Steps To Reproduce

  1. Open the minimal reproduction.

    Code backup import { useEffect, useState } from "react"; export default function () { const [done, setDone] = useState(false); useEffect(() => { if (done) return; setDone(true); console.log("%cMust run once", "color:red"); }); return <>; }
  2. Open the console.
  3. Actual behavior: ā€œMust run onceā€ is logged twice.
  4. Expected behavior: ā€œMust run onceā€ is logged once.

Note on the initial rerender

The minimal reproduction relies on the setup running the effect an extra time to begin with. I donā€™t understand why it does that, but it enables the demo. We can side-step and have a slightly longer demo that does not rely on the initial extra render:

  1. Open the larger reproduction.

    Code backup import { useEffect, useReducer, useState } from "react"; let fastRendersRemaining = 0; export default function () { const [rerenderId, rerender] = useReducer((i) => i + 1, 0); useEffect(() => { if (fastRendersRemaining === 0) return; fastRendersRemaining -= 1; queueMicrotask(rerender); }); const [done, setDone] = useState(true); useEffect(() => { // console.log('Current values:', { rerenderId, done }); if (done) return; setDone(true); console.log("%cMust run once", "color:red"); }); return ( ); }
  2. Open the console.
  3. Click on ā€œRun demonstrationā€.
  4. Actual behavior: ā€œMust run onceā€ is logged six times.
  5. Expected behavior: ā€œMust run onceā€ is logged once.
spencersablan commented 1 month ago

Hey @denis-sokolov! šŸ‘‹ In your first example, it seems like the component is rerendering due to the <StrictMode> wrapper in index.js. This will cause each component, and any effects, to rerender an extra time. See the strict mode docs here.

As for your larger reproduction example, things get really interesting... Some things to note right off the bat:

So this is what I think is going on: when you click the button, it is rerendering the component. When the microtask is setup in the first useEffect, that's going to always take precedence over any state updates. So even though you're calling setState(true) in the second useEffect, its not actually getting executed until there are no more microtasks scheduled.

To demonstrate this, you can remove queueMicrotask and just call rerender(). This would cause React to batch the rerender() call with setDone(true), ensuring they both get executed during the first re-render. šŸ˜„