search

silvermine / videojs-chromecast

MIT License
148 stars 75 forks source link

fix: clean event listeners on player.dispose() (#97) #99

Closed dwassing closed 3 years ago

dwassing commented 3 years ago

Hello videojs-chromecast maintainers,

I humbly request for this fix to be added to the baseline of the chromecast plugin. This addresses the issue I found pertaining to memory leaks from the event listeners in the ChromecastSessionManager. Source for the problem and solution I came up with may be found here.

Reduced test case can be found here, to reproduce the error that this pull request fixes, open console manager, press mute button on the started player (this disposes the player), tab into another browser tab, tab back in, and you will see that the console produces errors because the event listeners have not properly detached.

I am still fairly new to web development, so I'm not entirely sure how else this can be solved than with the global variables introduced on lines 7 and 8. If you have a better solution, I would very much like to hear it.

Kind regards, Daniel

jthomerson commented 3 years ago

@dwassing thanks for the fix. I just pushed some changes to it. Your fix made the session and cast listeners a shared global between all instances of the session manager, which didn't seem right (although I can't be 100% sure since I don't currently have a way of testing it). I made them instance variables of the session manager.

Could you test that this fix still works? If so, ping me and @jimjenkins5. @jimjenkins5 can also review and then we can merge and cut a release.

dwassing commented 3 years ago

Hello @jthomerson, ( also @jimjenkins5 ) thank you for the quick response! Your change definitely checks out, thank you for pointing it out. I have been furiously testing the past twenty minutes and so far I cannot see any errors. Issue seems resolved.

Looking forward to a release with this fix!

Kind regards, Daniel

jimjenkins5 commented 3 years ago

This looks good to me. I tested it with my MVP of our VideoJS plugins.

Thanks @dwassing!

jthomerson commented 3 years ago

Thanks @dwassing and @jimjenkins5 !

Published in + @silvermine/videojs-chromecast@1.3.2