Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix listeners leak when using with componentDidCatch #52

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

SamyPesse
Copy link

@SamyPesse SamyPesse commented Aug 29, 2018

⚠️ 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.

export class Subscribe<Containers: ContainersType> extends React.Component<
SubscribeProps<Containers>,
SubscribeState
class SubscribeUpdater<Containers: ContainersType> extends React.Component<
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big fan of splitting the Subscribe component in two components, but because of the context API, it looked like the cleaner method to handle the lifecycle of the instances (subscribe to new instances, unsubscribe from previous one when it render, etc).

@SamyPesse
Copy link
Author

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...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
1 participant