Clean up async function in an useEffect React hook

Question

I have the following useEffect function and trying to find the best way to clean this up when the component unmounts.

I thought it would be best to follow the makeCancelable from the React docs, however, the code still executes when the promise is cancelled.

const makeCancelable = (promise) => {
  let hasCanceled_ = false;

  const wrappedPromise = new Promise((resolve, reject) => {
    promise.then(
      val => hasCanceled_ ? reject({isCanceled: true}) : resolve(val),
      error => hasCanceled_ ? reject({isCanceled: true}) : reject(error)
    );
  });

  return {
    promise: wrappedPromise,
    cancel() {
      hasCanceled_ = true;
    },
  };
};

//example useEffect
useEffect(() => {
  const getData = async () => {
    const collectionRef_1 = await firestore.collection(...)
    const collectionRef_2 = await firestore.collection(...)
    if (collectionRef_1.exists) {
      //update local state
      //this still runs!
    }
    if (collectionRef_2.exists) {
      //update local state
      //and do does this!
    }
  }
  const getDataPromise = makeCancelable(new Promise(getData))
  getDataPromise.promise.then(() => setDataLoaded(true))
  return () => getDataPromise.cancel()
}, [dataLoaded, firestore])

I have also tried const getDataPromise = makeCancelable(getData) without any luck. The code executes fine, just doesn't clean up correctly when the component unmounts.

Do I need to also cancel the two await functions?

makeCancelable(new Promise(getData)) I'd say this should be makeCancelable(getData()) notice function call — Yury Tarabanko
– Yury Tarabanko, Commented Apr 24, 2020 at 18:02

Rashomon · Accepted Answer · 2020-04-24 18:45:13Z

7

In your makeCancelable function you are just checking the value of hasCanceled_ after the promise has finished (meaning getData has already executed entirely):

const makeCancelable = (promise) => {
  let hasCanceled_ = false;

  const wrappedPromise = new Promise((resolve, reject) => {
    // AFTER PROMISE RESOLVES (see following '.then()'!), check if the 
    // react element has unmount (meaning the cancel function was called). 
    // If so, just reject it
    promise.then(
      val => hasCanceled_ ? reject({isCanceled: true}) : resolve(val),
      error => hasCanceled_ ? reject({isCanceled: true}) : reject(error)
    );
  });

  return {
    promise: wrappedPromise,
    cancel() {
      hasCanceled_ = true;
    },
  };
};

Instead, in this case I would recomend you to go for a simpler and more classic solution and use a isMounted variable to create the logic you want:

useEffect(() => {
  let isMounted = true
  const getData = async () => {
    const collectionRef_1 = await firestore.collection(...)
    const collectionRef_2 = await firestore.collection(...)
    if (collectionRef_1.exists && isMounted) {
      // this should not run if not mounted
    }
    if (collectionRef_2.exists && isMounted) {
      // this should not run if not mounted
    }
  }
  getData().then(() => setDataLoaded(true))
  return () => {
    isMounted = false
  }
}, [dataLoaded, firestore])

edited Apr 24, 2020 at 18:45

answered Apr 24, 2020 at 17:57

Rashomon

6,8924 gold badges41 silver badges84 bronze badges

Sign up to request clarification or add additional context in comments.

5 Comments

Charklewis Over a year ago

Ah I see what you mean now by checking it after, rather than cancelling it during. I will try to rework my useEffect to get this working. React states that doing an isMounted check is an anti-pattern to trying to avoid doing this.

Rashomon Over a year ago

Maybe these docs only apply to class components? Because you can see Facebook implementing this "antipattern" with hooks in the Relay docs relay.dev/docs/en/experimental/…

Charklewis Over a year ago

I think you could be right. The example in the docs is using classes!

Rashomon Over a year ago

BTW, found the same pattern (namedignore instead of isMounted) in the own React Docs: reactjs.org/docs/…

Charklewis Over a year ago

Awesome, I have adjusted my function to use this approach and it works great. I have added a link in the question to where react stated this was an anti-pattern. But as you mentioned, it's becoming more clear that this was just for classes.

Collectives™ on Stack Overflow

Clean up async function in an useEffect React hook

1 Answer 1

5 Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

1 Answer 1

5 Comments

Your Answer

Sign up or log in

Post as a guest

Related