Update to remove the "setState on unmounted component" warning

[gaearon]

Overview

We've removed a warning when you call setState on an unmounted component.

Background

Previously, if you call setState on an unmounted component, you would see:

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.

Unfortunately, this warning was widely misunderstood and is somewhat misleading.

The original case against which it was meant to protect looks like this:

useEffect(() => {
  function handleChange() {
     setState(store.getState())
  }
  store.subscribe(handleChange)
  return () => store.unsubscribe(handleChange)
}, [])

Here, if you forget the unsubscribe call in the effect cleanup, you'll have a memory leak.

Why the warning didn't work well

In practice, the case above is uncommon to write. This is much more common in product code:

async function handleSubmit() {
  setPending(true)
  await post('/someapi') // component might unmount while we're waiting
  setPending(false)
}

Here, too, the warning will fire. However, in this case the warning is misleading.

There is no actual memory leak here:

1. The Promise will likely resolve soon, after which it can be garbage-collected.
2. Even if it wasn't resolved soon, the warning is not actionable if you use Promises.

Typically, people "suppress" the warning by writing code like this:

let isMountedRef = useRef(false)
useEffect(() => {
  isMountedRef.current = true
  return () => {
    isMountedRef.current = false
  }
}, [])

async function handleSubmit() {
  setPending(true)
  await post('/someapi')
  if (!isMountedRef.current) {
    setPending(false)
  }
}

However, this fix does not actually solve any "memory leaks". It just suppresses the warning. As mentioned earlier, there's also no actual leak here anyway. Waiting for a Promise is temporary, and there's nothing holding onto the component indefinitely.

The workaround is worse than the original problem

The workaround above is very common. But not only is it unnecessary, it also makes things a bit worse:

• In the future, we'd like to offer a feature that lets you preserve DOM and state even when the component is not visible, but disconnect its effects. With this feature, the code above doesn't work well. You'll "miss" the setPending(false) call because isMountedRef.current is false while the component is in a hidden tab. So once you return to the hidden tab and make it visible again, it will look as if your mutation hasn't "come through". By trying to evade the warning, we've made the behavior worse.

• In addition, we've seen that this warning pushes people towards moving mutations (like POST) into effects just because effects offer an easy way to "ignore" something using the cleanup function and a flag. However, this also usually makes the code worse. The user action is associated with a particular event — not with the component being displayed — and so moving this code into effect introduces extra fragility. Such as the risk of the effect re-firing when some related data changes. So the warning against a problematic pattern ends up pushing people towards more problematic patterns though the original code was actually fine.

Removing the warning

To sum up, we're removing this warning. The case for which it's relevant (subscriptions) is not very common in the product code, and is typically encapsulated in custom Hooks and libraries. The case that most product developers encounter is the one where this warning is not only useless/misleading, but also pushes developers to worse solutions while trying to suppress the warning. Hopefully, this change will reduce the confusion, and let you remove a bunch of isMounted checks from your code.

[eps1lon]

Would it make sense to keep the warning if we get an update from un unmounted component during an event? Manual event handlers (element.addEventListener) are probably also fairly common and it's easy to mess up their cleanup (e.g. options not matching).

Though I can understand that it's better to not warn at all instead of only warning sometimes. Manual event handlers are probably also mostly library territory.

[gaearon]

That's an interesting idea. Are there any false positive cases you could think of? I can't think of one yet...

[eps1lon]

At least not related to subscriptions for events. As I understood it, window.event is already considered when deciding on a lane so I would just re-use that logic.

I only know of a false-positive for media queries (https://github.com/mui-org/material-ui/pull/17315/files) but those don't dispatch events as far as I know so we shouldn't get false-positives there.

[gaearon]

For now I think we'll leave it as is. If this keeps coming up as a problem in the future, we can consider this.