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

feat: allow plugin deregistration from videojs #5273

Merged
merged 1 commit into from
Jul 24, 2018

Conversation

brandonocasey
Copy link
Contributor

@brandonocasey brandonocasey commented Jun 22, 2018

Currently you have to get the base plugin class and then deregister from there, which is a bit weird since you can directly register a plugin from videojs. This pr allows plugin de-registration from videojs as well.

@gkatsev
Copy link
Member

gkatsev commented Jun 22, 2018

My main concern is about the weird state we can get into with the way deregistration is currently set up: if you have a registered plugin, invoke it, and then deregister. Should we dispose active instances of the plugin? or just let it be?

@gkatsev
Copy link
Member

gkatsev commented Jun 22, 2018

Also, I didn't realize that the deregisterPlugin was available on the Plugin object, I would've preferred it not be visible at all

@brandonocasey
Copy link
Contributor Author

I think deregisterPlugin should only stop future players from getting said plugin. Plugins that are already on a player should probably be dealt with manually.

@gkatsev
Copy link
Member

gkatsev commented Jun 22, 2018

Maybe that's fine? Let's leave this to marinate for a bit.

@thijstriemstra
Copy link
Contributor

An addition to the plugin documentation illustrating usage of this feature would be helpful.

@thijstriemstra
Copy link
Contributor

Im not native english speaker but unregister instead of deregister sounds better to me.

@gkatsev
Copy link
Member

gkatsev commented Jul 20, 2018

They're synonyms but as this person outlines https://english.stackexchange.com/questions/25931/unregister-vs-deregister
deregister has the slight connotation that something was previously registered where-as unregistered can imply that it was never registered in the first place.

Also, given that Plugin.deregisterPlugin already exists, I would say we should just stick with it.

@gkatsev gkatsev merged commit 31a0bac into master Jul 24, 2018
@gkatsev gkatsev deleted the feat/deregister-from-videojs branch July 24, 2018 20:26
gkatsev pushed a commit that referenced this pull request Jul 26, 2018
This exposes `deregisterPlugin` on the videojs alongside `registerPlugin`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants