Skip to content

Fix listeners leak when using with componentDidCatch #52

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.

Sign up for GitHub

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

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

Fix listeners leak when using with componentDidCatch #52

Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
265d031
Subscribe in componentDidMount
SamyPesse Aug 29, 2018
cf718b9
Remove call to unscribe happening during the render
SamyPesse Aug 29, 2018
cb525d4
Split the subscribe logic from the context consumer
SamyPesse Aug 29, 2018
4bbb9a6
Fix errors
SamyPesse Aug 29, 2018
5fbe464
Fix typechecking
SamyPesse Aug 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 45 additions & 23 deletions src/unstated.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,25 +64,46 @@ export type SubscribeProps<Containers: ContainersType> = {
) => Node
};

type SubscribeState = {};
type SubscribeUpdaterProps<Containers: ContainersType> = {
instances: Array<ContainerType>,
children: (
...instances: $TupleMap<Containers, <C>(Class<C> | C) => C>
) => Node
};
type SubscribeUpdaterState = {};

const DUMMY_STATE = {};

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

@SamyPesse SamyPesse Aug 29, 2018

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

SubscribeUpdaterProps<Containers>,
SubscribeUpdaterState
> {
state = {};
instances: Array<ContainerType> = [];
unmounted = false;

componentDidMount() {
this._subscribe(this.props.instances);
}

componentWillUnmount() {
this.unmounted = true;
this._unsubscribe();
this._unsubscribe(this.props.instances);
}

componentDidUpdate(prevProps: SubscribeUpdaterProps<Containers>) {
this._unsubscribe(prevProps.instances);
this._subscribe(this.props.instances);
}

_unsubscribe() {
this.instances.forEach(container => {
_subscribe(instances: Array<ContainerType>) {
instances.forEach(container => {
container.unsubscribe(this.onUpdate);
container.subscribe(this.onUpdate);
});
}

_unsubscribe(instances: Array<ContainerType>) {
instances.forEach(container => {
container.unsubscribe(this.onUpdate);
});
}
Expand All @@ -97,20 +118,26 @@ export class Subscribe<Containers: ContainersType> extends React.Component<
});
};

render() {
return this.props.children.apply(null, this.props.instances);
}
}

export class Subscribe<Containers: ContainersType> extends React.Component<
SubscribeProps<Containers>
> {
_createInstances(
map: ContainerMapType | null,
containers: ContainersType
): Array<ContainerType> {
this._unsubscribe();

if (map === null) {
throw new Error(
'You must wrap your <Subscribe> components with a <Provider>'
);
}

let safeMap = map;
let instances = containers.map(ContainerItem => {
return containers.map(ContainerItem => {
let instance;

if (
Expand All @@ -127,25 +154,20 @@ export class Subscribe<Containers: ContainersType> extends React.Component<
}
}

instance.unsubscribe(this.onUpdate);
instance.subscribe(this.onUpdate);

return instance;
});

this.instances = instances;
return instances;
}

render() {
return (
<StateContext.Consumer>
{map =>
this.props.children.apply(
null,
this._createInstances(map, this.props.to)
)
}
{map => (
<SubscribeUpdater
instances={this._createInstances(map, this.props.to)}
>
{this.props.children}
</SubscribeUpdater>
)}
</StateContext.Consumer>
);
}
Expand Down