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

Automatic cleanup of listeners on other components #1588

Closed
wants to merge 3 commits into from

Conversation

heff
Copy link
Member

@heff heff commented Oct 16, 2014

Implemented half of #1544. Now instead of using:

player.on('some-event', vjs.bind(this, fn));

You can use:

this.on(player, 'some-event', fn);

Using the second form will automatically clean up the listener whenever either the player or the component is disposed. And 'player' can be replaced with any other component.

When implementing this in existing components I found a few places we weren't removing listeners on dispose, and they should be fixed now with this.

UPDATED to include other elements, e.g.

this.on(document, 'some-event', fn);

…mponents. closes videojs#1544

Automatically cleans up listeners when either component is diposed.
Also changed how a component was determined in on() so mock players could be used
…n' function.

    myComponent.on(document, 'eventType', fn);
    myComponent.off(document, 'eventType', fn);
    myComponent.one(document, 'eventType', fn);
@heff
Copy link
Member Author

heff commented Oct 20, 2014

I added the option to attach listeners to other elements. component.on(someElement, 'some-event', fn);

With regular elements there's potential here to create a memory leak, because if you remove an element from the DOM without removing the listener, there will still be a reference to the element until the component is disposed. So originally @mmcc and I discussed an API like component.onDoc() specifically, because the document is permanent. But thinking through it, we need to be good about cleaning up listeners either way, so allowing on() to be used on elements doesn't make that any more complicated. And with elements you can still use vjs.trigger(someElement, 'dispose') to clean up these listeners before removing the element from the DOM.

I'd love to get a second set of eyes on this if anyone's up for it.

@hacfi
Copy link

hacfi commented Oct 21, 2014

👍 very nice!

@heff heff closed this in 4b818d9 Oct 28, 2014
jgubman added a commit to jgubman/video.js that referenced this pull request Oct 29, 2014
* 'stable' of https://github.com/videojs/video.js: (22 commits)
  Added line to changelog for 4.10.1
  Release 4.10.1
  Removed alert...
  Release 4.10.0
  @heff turned on the custom html controls for touch devices. closes videojs#1617
  updated options to include sans-children, and included self-hosted font blurb
  added CORS information to tracks guide. Fixes videojs#837
  doc updates + readme quick start
  updated options guide to include components
  @heff Added the ability to set options for child components directly in the parent options. closes videojs#1599
  @DevGavin added a Simplified Chinese translation. closes videojs#1593
  @mmcc fixed an issue with the VolumeButton assuming it was vertical by default. closes videojs#1592
  @heff enhanced the event listener API to allow for auto-cleanup of listeners on other componenets and elements. closes videojs#1588
  @mmcc fixed an issue where errors on source tags could get missed. closes videojs#1575
  Added doc for remaining time and removed onWaitEnd doc
  maybe actually check for keyLocation
  Update languages.md
  @heff updated the poster to use CSS styles to display; fixed the poster not showing if not originally set. closes videojs#1568
  Release 4.9.1
  Bumped to videojs-swf v4.5.1 to fix a data sanitization issue. closes videojs#1587
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants