Best practices for async await when retrieving data from firestore

Question

I had this error before

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.

This is my useEffect where I retrieve data from firestore and it does remove the error:

 useEffect(async () => {
    let isMounted = true;
    const querySnapshot = await getDocs(collection(db, "product"));
    const arr = [];
    querySnapshot.forEach((doc) => {
      arr.push({
        ...doc.data(),
        id: doc.id,
      });
      if (isMounted) setProduct(arr);
    });
    return () => {
      isMounted = false;
    };
  }, []);

Is the useEffect alright or will this cause any problems in the future? And am I mounting this right?

‘setProduct’ is a setState, right? If yes, your implementation looks good. — Shreshth, Feb 19 '22 at 03:10

score 2 · Answer 1 · answered Feb 19 '22 at 03:26

Ths look okay. But it's best to extract the fetching logic into a separate function instead of having the entire logic in the body of the useEffect. It gets easy to re-use the logic for functionality like retrying if the request failed on mount.

I would do something like this...

Note: I removed the isMounted as the effect with be run when the component is mounted.
Wrap the function in an async function(named or anonymous) inside the useEffect and run it immediately.

// Re-usable logic for stuff like reload/retry
const fetchProduct = async () => {
  const querySnapshot = await getDocs(collection(db, "product"));
  const arr = [];
  querySnapshot.forEach((doc) => {
    arr.push({
      ...doc.data(),
      id: doc.id,
    });
  });
  setProduct(arr); // Moved this out of the loop to be run once.
}

useEffect(async () => {
  (async () => {
    await fetchProduct();
    // await other async operations here.
  })(); // This immediately runs the func async.
}, []);

score 2 · Accepted Answer · answered Feb 19 '22 at 03:51

Looking at your current code, you call useEffect with an async function that uses await. Because you are using an async function, the unsubscriber function you are returning to useEffect is wrapped in a Promise instead of being the raw callback itself. This means that the useEffect never sees that the return value is a callback function in time for it to be useful.

The solution to this is to make sure the function passed into the useEffect is not async.

useEffect(() => {
  let isMounted = true;

  const doFetch = async () => {
    const querySnapshot = await getDocs(collection(db, "product"));
    const arr = [];
    querySnapshot.forEach((doc) => {
      arr.push({
        ...doc.data(),
        id: doc.id,
      });
      if (isMounted) setProduct(arr);
    });
  };

  doFetch() // start the async work
    .catch((err) => {
      if (!isMounted) return; // unmounted, ignore.
      // TODO: Handle errors in your component
      console.error("failed to fetch data", err);
    });

  return () => {
    isMounted = false;
  };
}, []);

There are useAsyncEffect implementations that allow you to skip the setState() and useEffect() boilerplate that you are currently using. I have detailed these in similar answers here and here.

Thank you, can I also add a loading with this `if (isMounted) setProduct(arr); setLoading(true);` or should I wrap both the `product` and `setLoading` with the `if`? — JS3, Feb 23 '22 at 07:13
@PennieCorhon Yes, you will also need to protect `setLoading` by the same `if` statement. However, I would instead recommend inverting the condition like I did with the `catch` handler by using `if (!isMounted) return; // unmounted, ignore.` and then continue on with your state changes below it (`setProduct`, `setLoading`, etc.) This is known as "failing fast". — samthecodingman, Feb 23 '22 at 12:14

Best practices for async await when retrieving data from firestore

2 Answers2

Linked