3
\$\begingroup\$

In React, it is quite common for me to have something similar to the following.

async componentDidMount() {
    try {
        this.setState({ isLoading: true })
        const { data } = await axios.get('...')
        this.setState({ data })
    } catch(error) {
         handleError(error)
    } finally {
        this.setState({ isLoading: false })
    }
}

This is great because the cleanup code (isLoading: false) is DRY. That is, until I try to cancel network requests on unmount. That code would look like this:

async componentDidMount() {
    try {
        this.axiosCancelTokenSource = axios.CancelToken.source()
        this.setState({ isLoading: true })
        const { data } = await axios.get('...', {
            cancelToken: this.axiosCancelTokenSource.token,
        })
        this.setState({ data })
    } catch(error) {
         if (axios.isCancel(error)) return
         handleError(error)
    } finally {
        this.setState({ isLoading: false })
    }
}
componentWillUnmount() {
    if (this.axiosCancelTokenSource) this.axiosCancelTokenSource.cancel()
}

The problem with this is that it will setState after the component unmounts, which React will warn againts.

As far as I see it, these are my options for dealing with this:

  1. Ignore the warning. React gives a warning when you setState after unmount because it indicates a memory leak (in this case, the lingering network request, if not cancelled). If the network request is cancelled, there is still a setState after unmount, but just to set a flag. There is no more lingering network request. It should be safe to ignore the warning in this case, but it doesn't feel right.

  2. Check what error was thrown in the finally block and add the same if statement as the catch block. This seems incredibly hacky and would require extra code to save the error from the catch block.

  3. Check if the component is mounted in the finally block. This is also hacky and requires boilerplate code to update a this.isMounted flag.

  4. Put the cleanup code at the end of try and after the condition in catch. This is not DRY. Humans are also very forgetful; I cannot count how many times I have forgotten to set isLoading = false in catch.

  5. Define a cleanup() function before the try and call it in try and catch. This is a decent option, but requires extra function calls, making it harder to follow.

So far, it looks like the first or fifth options are best, depending on how much you care about seeing warning messages. Am I missing any good options?

\$\endgroup\$
  • \$\begingroup\$ IMHO 5th option but that's just personal opinion \$\endgroup\$ – Pimgd Apr 29 at 9:20
1
+50
\$\begingroup\$

Finally only for unconditional execution

I am not a React user, and as such I may be missing something unique to the React way.

It seams very strange that you use a finally block to execute code you don't want to guarantee execution.

As far as I see it, these are my options for dealing with this:

You list 5 options... Why not a 6th?

  1. Remove the finally { ... } block setting the is loading state after the try catch and having the catch return if unmounted.

If I ignore React the "Idomatic way to ignore finally block..." is to remove it.

eg

async componentDidMount() {
    try {
        this.axiosCancelTokenSource = axios.CancelToken.source()
        this.setState({ isLoading: true })
        const { data } = await axios.get('...', {
            cancelToken: this.axiosCancelTokenSource.token,
        })
        this.setState({ data })
    } catch(error) {
         if (axios.isCancel(error)) return
         handleError(error)
    }
    this.setState({ isLoading: false })
}

Catch only known exceptions

try ... catch should only be used to wrap code that is known to throw a known set of exceptions (in this case network, data, or forced exceptions related to axios.get).

Wrapping all code automatically in a try catch means that it is possible to catch unknown exceptions (AKA BUGS) effectively hiding/obscuring the erroneous behavior during the development cycle.

Example

Removing the try from around known safe code, catching only exceptions related to the functions role.

// pseudo code as example only
async mount() {
    loading = (isLoading = true) => this.setState({isLoading});
    cancelToken = axios.CancelToken.source();
    loading();
    try {
        this.setState({data: (await axios.get("...", {cancelToken})).data});
    } catch (e) {
         if (axios.isCancel(e)) { return }
         handleError(e);
    } 
    loading(false);
}
\$\endgroup\$
  • \$\begingroup\$ Can't believe I didn't think of this. \$\endgroup\$ – Marcel Apr 30 at 17:46
2
\$\begingroup\$

Ignore the warning. React gives a warning when you setState after unmount because it indicates a memory leak (in this case, the lingering network request, if not canceled). If the network request is canceled, there is still a setState after unmount, but just to set a flag. There is no more lingering network request. It should be safe to ignore the warning in this case, but it doesn't feel right.

Bad idea. Thing is, by the time the request will resolve, setState might be triggered on an unmounted component. And warning turns to error.

Check what error was thrown in the finally block and add the same if statement as the catch block. This seems incredibly hacky and would require extra code to save the error from the catch block.

Meh :\

Check if the component is mounted in the finally block. This is also hacky and requires boilerplate code to update a this.isMounted flag.

It's an Anti-pattern

enter image description here

Put the cleanup code at the end of try and after the condition in catch. This is not DRY. Humans are also very forgetful; I cannot count how many times I have forgotten to set isLoading = false in catch.

This

Define a cleanup() function before the try and call it in try and catch. This is a decent option, but requires extra function calls, making it harder to follow.

And that, are pretty much taking the same turn with different gear.

Suggestion from React Blog

If you use ES6 promises, you may need to wrap your promise in order to make it cancelable.

const cancelablePromise = makeCancelable(
  new Promise(r => component.setState({...}))
);

cancelablePromise
  .promise
  .then(() => console.log('resolved'))
  .catch((reason) => console.log('isCanceled', reason.isCanceled));

cancelablePromise.cancel(); // Cancel the promise

Where makeCancelable was originally defined by @istarkov as:

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;
    },
  };
};
\$\endgroup\$
  • \$\begingroup\$ Your answer might be better if you provided at least a small part of the anti-pattern link in your own words around the line. \$\endgroup\$ – pacmaninbw Apr 30 at 0:43
  • \$\begingroup\$ Thanks for the answer! Under what circumstances will the "setState in unmounted component" throw an error not a warning? You say it's when the component is not mounted, but that's the whole point of the warning. I fail to see when it becomes an error. Also, where's the shame in considering an anti-pattern and deciding against it? \$\endgroup\$ – Marcel Apr 30 at 17:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.