react cleanup in useEffect

Question

How can I clean up this useEffect correctly?

  useEffect(() => {
    if (inView && !fetching.current) {
      setLoading(true);
      fetching.current = true;
      delay(delayMs).then(async () => {
        const country = await fetchData();
        setMoreCountriesFunc(country);
        fetching.current = false;
      });
    }
    return () => {};
  }, [inView, delayMs, setMoreCountriesFunc, fetchData]);

fetchData() is basically a function where I make fetch request

console error:

index.js:1 Warning: Can't perform a React state update on an unmounted component.
This is a no-op, but it indicates a memory leak in your application. To fix, cancel all 
subscriptions and asynchronous tasks in a useEffect cleanup function.

Answer 1

Looking at your problem it looks like the issue is setMoreCountriesFunc.

For your issue, you can do two things:

1. Abort the API call(not supported in older browsers). Check AbortController

   const fetchData = (signal) => fetch('/api/some-url', {signal}).catch((e) => { // catch AbortError });

   useEffect(() => {
         let controller;
         if (inView && !fetching.current) {
             setLoading(true);
             fetching.current = true;
   
             controller = new AbortController();
             const { signal } = controller;
   
             delay(delayMs).then(async () => {
                 const country = await fetchData(signal);
                 setMoreCountriesFunc(country);
                 fetching.current = false;
             });
        }
        return () => {
            controller?.abort();
        };
    }, [inView, delayMs, setMoreCountriesFunc, fetchData]);
   

2. Do not set the state after unmount and ignore.

    useEffect(() => {
        if (inView && !fetching.current) {
            setLoading(true);
            fetching.current = true;
            delay(delayMs).then(async () => {
                const country = await fetchData();
                if(fetching.current) {
                    setMoreCountriesFunc(country);
                }
                fetching.current = false;
            });
       }
       return () => {
           fetching.current = false;
       };
   }, [inView, delayMs, setMoreCountriesFunc, fetchData]);
   

Benefit of using approach 2 over 1 is that you can refactor your effect to allow the last API call and abort all other pending API calls, which means if the API call is fired multiple times, you'll safely and surely get the correct response for the last API call.

Answer 2

Async code in an effect means it could run after the component gets unmounted.

If what bothers you is just the warning about the setState when unmounted, this should go away with React 18 version. (I can't have a link to that, if anybody bothers to edit!)

If you want to avoid async code to run in an effect after component gets unmounted, what you could do is cancel the promises:

useEffect(() => {
    if (inView && !fetching.current) {
      setLoading(true);
      fetching.current = true;
      let fetchPromise;
      try {
        const delayPromise = delay(delayMs).then(async () => {
          const country = await fetchData();
          setMoreCountriesFunc(country);
          fetching.current = false;
        });
      catch() {
        // Manage abortion case if necessary
      }
      return () => {
        delayPromise.cancel()
        fetchPromise && fetchPromise.cancel()
      } 
    }
  }, [inView, setMoreCountriesFunc, fetchData]);

That would need both your promises to support cancel operation. For fetch operations, the promise returned by your fetchData should probably implement AbortController as explained here, and the delay also.

This is what is suggested in the react docs here

Otherwise, IMHO you can always have a ref that would hold the mount status and use this as a condition to stop running the effect.

You could refactor as following:

const isMountedRef = useRef(true)

useEffect(() => () => { isMountedRef.current = false }, [])

useEffect(() => {
    if (inView && !fetching.current) {
      setLoading(true);
      fetching.current = true;
      delay(delayMs).then(async () => {
        // Avoid useless fetch
        if (!isMountedRef.current) return 
        const country = await fetchData();
        // Avoid setting state on unmounted component
        if (!isMountedRef.current) return 
        setMoreCountriesFunc(country);
        fetching.current = false;
      });
    }
  }, [inView, setMoreCountriesFunc, fetchData]);

Note: why holding the loading state in both a ref and a state? The later should be enough if you need a re-render, or the former if you don't?