unstated icon indicating copy to clipboard operation
unstated copied to clipboard

Fix listeners leak when using with componentDidCatch

Open SamyPesse opened this issue 7 years ago • 1 comments

⚠️ This PR is not finished, neither tested. I'm opening it now to gather feedback and suggestions of solutions that I may have missed.

I've investigated a bug in my application coming from the use of unstated and componentDidCatch.

At some point container.setState was returning a never-fulfilled promise. React was logging:

warning.js:33 Warning: Can't call setState (or forceUpdate) 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 the componentWillUnmount method.
    in Subscribe
    in Fetch
    in FetchBoundary (created by ViewTable)
    in ViewTable (created by SecondaryView)
    in SecondaryView

When an error is thrown during the render phase, the componentWillUnmount is never called, and the component doesn't unsubscribe from the container.

When calling setState, since the component for the listener is unmounted, onUpdate returns a never-fulfilled promise.

Potential solution: I'm experimenting moving the subscribe calls to a safe place componentDidMount (which happens during the commit phase, see this comment that explains it).

Any way, I'd love to get your feedback on what could be the best approach to fix this 🙌


TLDR: When using unstated with componentDidCatch, it leaks listeners, and prevent setState from being fulfilled.

SamyPesse avatar Aug 29 '18 21:08 SamyPesse

This solution is not working, because it changes the order of listeners (child component subscribe before the parent does).

I'm gonna investigate other solutions...

SamyPesse avatar Aug 30 '18 12:08 SamyPesse