Allow to use custom Player class

[adam187]

Description

Allow to use custom player class

Specific Changes proposed

This change will allow to use custom player class. In version 4 new player instance was created from vjs.Player https://github.com/videojs/video.js/blob/release/4.2.0/src/js/core.js#L52 so it was possible to override Player witch custom class. Right now it's not possible since player is created directly from Player import. Using Component.getComponent('Player') will alllow to use custom player class that can be registered via Component.registerComponent same as current player is registered right now https://github.com/videojs/video.js/blob/master/src/js/player.js#L2979

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

[adam187]

@gkatsev Use case for this PR.

I`m currently working on upgrading video.js from version 4 to version 5 in a project.
This project is using videojs with some custom functionally on top, some of this custom functionality is in custom player class.

Player was extended and replaced with code like this:

const {Player} = videojs;
videojs.Player = Player.extend({
    init(player, opts, ready) {
       Player.call(this, player, options, ready);
    }
});

Main reason for this PR is ability to use custom player class via standard videojs function and and auto load feature.

In this project player is embedded in many places and i would rather avoid situation in which update player would involve modifying code to create player instance.

At first i tried to replace only videojs function with custom version that uses player component (like the one in PR) but it didn't solve all my problems since autoload was still using original videojs function https://github.com/videojs/video.js/blob/master/src/js/video.js#L125.

[gkatsev]

Thanks for the details, @adam187. Seems like this is a simple enough change for a lot of benefit.

@videojs/core-committers thoughts?

[heff]

Yeah, I don't see any down sides.

[gkatsev]

LGTM.

[brandonocasey]

LGTM

[misteroneill]

LGTM

[gkatsev]

Would you be able to rebase this?
Otherwise, I can do it as part of merging.

[adam187]

@gkatsev Done.

[gkatsev]

Awesome, thanks!

[gkatsev]

Just tested it locally. One thing that I realized could potentially be an issue is that you're no longer able to access the original Player object.
Probably only an issue if a plugin is changing out the Player but you only want to apply the plugin to one video and not another video on a single page.

[adam187]

@gkatsev Well, in my use case i'm changing player before any instance of videojs is initialized. But isn't this an potential issue with other components replaced via registerComponent from plugin?

[gkatsev]

@adam187 Yup, it's definitely an issue. Though, the Player component is the main piece of the videojs and we've previously not officially recommended replacing existing components via registerComponent. So, just want to make sure we think through any implications and decide whether it's something we should address or just something that could be documented or maybe just something to be aware of.

[adam187]

@gkatsev Yes, i`m fully understand your your concern.

We could throw error when registerComponent is called with Player name and there already is player with instances.

[gkatsev]

hm...
@videojs/core-committers thoughts on the above question?

[misteroneill]

I think that throwing would be an option in the case you outlined @gkatsev. It's not a breaking change, either, because this wasn't previously allowed.

Perhaps it should throw if you're attempting to registerComponent('Player', Foo) after any player has been created? In that case, you can only use this feature in the way @adam187 is using it - registering a new player component before creating any players.

[gkatsev]

Throwing early is probably good, and we could always loosen it later if we necessary.

@adam187 would you be able to add that in? Also, looks like there's a conflict, probably because of the new hooks feature.

[adam187]

@gkatsev Take a look.

[gkatsev]

@adam187 thanks!