2

When state is in a hook it can become stale and leak memory:

function App() {
  const [greeting, setGreeting] = useState("hello");

  const cb = useCallback(() => {
    alert("greeting is " + greeting);
  }, []);

  return (
    <div className="App">
      <button onClick={() => cb()}>Click me</button>
      <p>
        Click the button above, and now update the greeting by clicking the one
        below:
      </p>
      <button onClick={() => setGreeting("bye")}>
        Update greeting
      </button>
      <p>Greeting is: {greeting}</p>
      <p>
        Now click the first button again and see that the callback still has the
        old state.
      </p>
    </div>
  );
}

Demo: https://codesandbox.io/s/react-hook-stale-datamem-leak-demo-9pchk

The problem with that is that we will run into infinite loops in a typical scenario to fetch some data if we follow Facebook's advice to list all dependencies always, as well as ensure we don't have stale data or memory leaks (as the example showed above):

const [state, setState] = useState({
  number: 0
});

const fetchRandomNumber = useCallback(async () => {
  if (state.number !== 5) {
    const res = await fetch('randomNumber');
    setState(v => ({ ...v, number: res.number }));
  }
}, [setState, state.number]);

useEffect(() => {
  fetchRandomNumber();
}, [fetchRandomNumber]);

Since Facebook say we should list fetchRandomNumber as a dependency (react-hooks/exhaustive-deps ESLint rule) we have to use useCallback to maintain a reference, but it regenerates on every call since it both depends on state.number and also updates it.

This is a contrived example but I've run into this many times when fetching data. Is there a workaround for this or is Facebook wrong in this situation?

Dominic
  • 62,658
  • 20
  • 139
  • 163
  • 1
    `fetchSomething` hasn't changed tho, has it? It's not state, it's just a method you're calling, so why are you listing it in the dependency array? Just use an empty array to use the effect once. – Andy Aug 22 '19 at 10:32
  • I agree but Facebook team think all deps should always be listed and by default it's a lint error: https://github.com/facebook/create-react-app/issues/6880 – Dominic Aug 22 '19 at 10:37
  • [I think you're safe to use the empty array in this case](https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies) because `fetchSomething` doesn't rely on anything from the component scope (that I can see). – Andy Aug 22 '19 at 10:49
  • That unfortunately not true, since state is read it will refer to an old value (mem leak too) unless the state is a primitive - see https://codesandbox.io/s/admiring-ganguly-9pchk – Dominic Aug 22 '19 at 11:03
  • Updated link: https://codesandbox.io/s/react-hook-stale-datamem-leak-demo-9pchk – Dominic Aug 22 '19 at 11:11

2 Answers2

3

Use the functional form of the state setter: 

const fetchData = useCallback(async () => {
  const res = await fetch(`url?page=${page}`);
  setData((data) => ([...data, ...res.data]));
  setPage((page) => page + 1);
}, [setData, setPage]);

Now you don't need data and page as your deps

You can also use a ref to run the effect only on mount : 

  const mounted = useRef(false);

  useEffect(() => {
    if(!mounted.current) {
      fetchSomething();
      mounted.current = true;
    }

    return () => { mounted.current = false }
  }, [fetchSomething]);

And 

const fetchSomething = useCallback(async () => {
  ...
}, [setData, setPage, data, page]);
Mohamed Ramrami
  • 12,026
  • 4
  • 33
  • 49
  • This works for setting, but not for reading state (`page` should still be passed into a dep). If the state is a non-primitive and not added to deps it will reference the wrong thing if the state updates to a new object or array (and also be a memory leak) – Dominic Aug 22 '19 at 10:38
  • I updated my answer with an example using a ref, this will prevent the infinite loop. This works only for the on mount case – Mohamed Ramrami Aug 22 '19 at 11:24
  • Hm that's an interesting idea thanks, I guess it can be extracted to a re-usable hook too. I'm leaning towards facebook being wrong in this case, but this is useful anyway if we have to use a hook for another reason and don't want it to be called again and again by a useEffect – Dominic Aug 22 '19 at 11:34
0

fetchSomething is not a dependency here. You don't want to retrigger the effect, you only cause it once when the component mounts. Thats what useEffect(() => ..., []) is for.

Jonas Wilms
  • 132,000
  • 20
  • 149
  • 151
  • 1
    I've rephrased the question a bit clearer - Facebook adds an ESLint rule called `react-hooks/exhaustive-deps` which you can't globally turn off. They think any variables/functions/state/props not external to the component should always be listed in deps – Dominic Aug 22 '19 at 11:29