-
Notifications
You must be signed in to change notification settings - Fork 55
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
enhance: Off removes all listeners for EventEmitter #227
Conversation
cc @tcard FYI in regards to the discussion at https://github.com/ably/ably-ios/pull/179/files#r51544655 |
} else if (Utils.isObject(listeners)) { | ||
/* events */ | ||
for (eventName in listeners) { | ||
if (listeners.hasOwnProperty(eventName) && Utils.isArray(listeners[eventName])) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not delete listeners[eventName]
if it is empty after the remove operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be deleted?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course. If an app for example continuously uses new randomly generated event names and then discards the listeners, the size of the events
object will grow indefinitely. It's easy to re-add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes of course. If an app for example continuously uses new randomly generated event names and then discards the listeners, the size of the events object will grow indefinitely. It's easy to re-add.
Ah, good point, I will make that change.
LGTM apart from the comment above. Also, someone needs to take these tests: https://github.com/ably/realtime/blob/master/test/util/eventemitter.js and migrate them. |
@paddybyers fixed with 4f50db9 |
LGTM |
4f50db9
to
3092796
Compare
See ably/docs#82
@paddybyers @SimonWoolf please review