React team has removed the warning on setting setState on an unmounted component

[payapula]

Hi @kentcdodds

I just noticed this today that react team removed the warning on setState on unmounted components - facebook/react#22114

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.

In the workshop material, (Exercise: 02, Extra credit 03) we may need to highlight this and give a link to this pull request Remove the warning for setState on unmounted components as it claims these are not really memory leak.

Just opening this here as a feedback.

[Aprillion]

IMHO the are actual (but insignificantly small) memory leaks were/are real, but the bigger problem was that 99.9% of workarounds to get rid of the warning (including the mounted ref in this workshop) could not do anything about any memory leaks, it only silenced the warning itself and made the code more complex...

the pattern can be still useful to have in our tool belts, but no idea what is the best scenario to explain it if dispatch / setState after unmount won't have to be guarded any more ¯\_(ツ)_/¯

if (isMounted) {
  doSomethingThatMakesSenseOnlyIfMounted()
}

[Aprillion]

...maybe starting some imperative animation for "payment confirmed" after 200 response from a POST request, that would only make sense if still mounted

and perhaps a counter-example to remove some imperative DOM event listeners - those should always be in the cleanup of a useEffect without any guarding, event listeners have to be removed unconditionally, especially if React is unmounting / already unmounted 😅

[kentcdodds]

Thanks @payapula! I'll be removing this from the workshop in the future :)

[aimanzaheb]

In your workshop material you mentioned "Also notice that while what we're doing here is still useful and you'll learn valuable skills"
function useSafeDispatch(dispatch) {
const mountedRef = React.useRef(false)

React.useEffect(() => {
mountedRef.current = true
return () => {
mountedRef.current = false
}
}, [])

return React.useCallback(
(...args) => (mountedRef.current ? dispatch(...args) : void 0),
[dispatch],
)
}

But As mentioned on the link reactwg/react-18#82
"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."

Please tell what should we do?

[kentcdodds]

Don't worry about it unless it becomes a problem in your app (it probably won't).