Fix leaking of GroupStore listeners in RoomList #1664
Conversation
lukebarnard1
changed the title
src/stores/GroupStore.js
Outdated
| @@ -110,6 +110,12 @@ export default class GroupStore extends EventEmitter { | |||
| this._fetchSummary(); | |||
| this._fetchRooms(); | |||
| this._fetchMembers(); | |||
|
|
|||
| return { | |||
There was a problem hiding this comment.
i'm a bit surprised that one can return a object like this from registerListener - is this just ignorance, or can we have a comment to explain what API this comes from?
There was a problem hiding this comment.
or is this adhoc of our own devising? in which case, it really should be jsdoc'd on the method as the return type
There was a problem hiding this comment.
This is fairly similar to the flux/utils.Store way of doing things, which returns
{ stop: () => {...} }
I shall leave a comment to explain. (The intention would be to At Some Point move to it actually being a Flux store.)
There was a problem hiding this comment.
ok. jsdoc would be good enough, imo: "This method returns an object which callers can use to invoke an unregister hook to..." or whatever.
There was a problem hiding this comment.
Awesome, I did both in the end 👍
|
lgtm modulo comment req |
Built on #1647