Auto-cleanup component event listeners on the document and other components

[heff]

We've had a few issues with event listeners not getting cleaned up appropriately when components are disposed of. A component will currently automatically clean up any listeners on its own element, however it won't automatically clean up listeners it has on the document or other components. After a discussion with @mmcc, we think it'd be good (and not too much magic) to clean up the other instances as well.

The current component event listener functions look like:

component.on('eventtype', func);
component.off('eventtype', func);

We would add the following APIs.

component.on(otherComponent, 'eventtype', func);
component.off(otherComponent, 'eventtype', func);

component.onDoc('eventtype', func);
component.offDoc('eventtype', func);

The new APIs would store records of the event listeners being added by a component (component X) to the doc/other components, and attempt to remove them when component X is disposed of.

In the case of other components, component X should listen for the dispose event of component Y, and remove any listeners on Y when it is disposed. This will prevent memory leaks where Y is prevented from being garbage collected.

We're assuming document will never be 'disposed'. The potential for memory leaks is why we can't make this possible for other generic elements. We couldn't know when an element is permanently removed from the document and release the reference to it.

Related Issues: #1475, #1476

[themicp]

I was about to open a new thread about this issue and thankfully, you were already on that.

I think that your proposed solution is not backwards compatible. Plugins that used to set listeners on other components with the old-fashioned way won't work until they get updated and this might break players using old plugins.

I am also concerned about the semantics of the setters. It starts with component.on(... and you assume that a listener in a component's event is going to be set, but it actually sets a listener on another component's event.

I was thinking if you could use the arguments.callee object on vjs.Component.on somehow to determine which constructor called it and then find the component that this constructor belongs to.

[heff]

I'm not sure I'm clear on your concerns, but the old way for adding event listeners to the component would not change.

// to have a component attach a listener to itself
component.on(eventType, listener);

// to have a component attach a listener to another component (new and optional)
component.on(otherComponent, eventType, listener);

In the on method we would check if the first argument is a string or a component. If it's a string it would continue to to work the same way as it always has. If it's a component we would then assume the second argument is type and the third argument is the listener.

Plugins that already set listeners on other components use this pattern:

otherComponent.on(eventType, vjs.bind(component, listener));

That pattern will still work as well.

[themicp]

What I was thinking about my first concern was the plugins that used to set the listeners using the current pattern will still have the mentioned problem and they won't get the fix until they update their setters.

About the semantics, I will give you a more clear example.
Let's say someone with no experience on videojs source tries to understand the functionality of a videojs module.
He starts reading the code and meets an event setter: myComponent.on(otherComponent, eventType, listener);.
The function that is called is myComponent.on() which seems like it sets an event listener on myComponent, on a first impression. What this pattern does, however, is setting an event listener on another component, the otherComponent, and I think that this might be confusing and not a really good practice.

Why use the on() function of a component A to set a listener on a component B?

[heff]

I don't think there's any way we can fix this in reverse for plugins without overcomplicating the vjs.on function, so it will have to just be a feature moving forward. It's a convenience feature anyway, so hopefully most plugins are already good about cleaning up listeners.

About the semantics

Interesting. It sounds like it'd be good to get some other opinions on this. To me it reads quite nicely.

componentA.on(componentB, 'click', doSomething);

ComponentA on ComponentB's click event should do something.

Especially when this is involved, which is probably more likely.

this.on(componentB, 'click', doSomething);

Why use the on() function of a component A to set a listener on a component B?

The main point of this feature is to clean up listeners that componentA puts on other components when componentA gets disposed. Currently we just use vjs.on, which is fine, but doesn't clean itself up.

[themicp]

Alright then, agreed. :)

[heff]

This feature was completed by #1588.