Possible incorrect logic with registering new Player

[victordidenko]

I'm not sure, but shouldn't there be some instead of every?

"If at least one player is not disposed yet"

video.js/src/js/component.js

Lines 2024 to 2032 in 62f3844

	// If we have players that were disposed, then their name will still be
	// in Players.players. So, we must loop through and verify that the value
	// for each item is not null. This allows registration of the Player component
	// after all players have been disposed or before any were created.
	if (players &&
	playerNames.length > 0 &&
	playerNames.map((pname) => players[pname]).every(Boolean)) {
	throw new Error('Can not register Player component after player has been created.');
	}

[welcome]

👋 Thanks for opening your first issue here! 👋

If you're reporting a 🐞 bug, please make sure you include steps to reproduce it. We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.
To help make it easier for us to investigate your issue, please follow the contributing guidelines.

[gkatsev]

Yes, I think you're correct that that should be a some and not every. Care to submit a PR? (Ideally, with a new test for it. Can use this as a template https://github.com/videojs/video.js/blob/main/test/unit/player.test.js#L2328-L2346)

Thanks!

[victordidenko]

Sure, but I wanted to ask about what is the reason of this limitation?
Because, funny enough, I found it while trying to workaround this error.

[gkatsev]

It's to try and make sure that all player instances on the page use the same player component. The idea being that if there are players that aren't the same player component, there may end up issues around it.

[victordidenko]

Yeah, I understand what this check is doing, I don't understand why :)
This exactly — "there may end up issues around it" — what issues?
My only guess is that this is because of Player.players map? It might be used somewhere else, maybe.
But if new Player class is a subclass of original Player class — they shares the same .players map, so, it shouldn't be a problem? 🤔
Maybe this check should be more comprehensive than current version?

[gkatsev]

Yes, this error is a bit theoretical. But, we essentially never want to get an issue opened because a user initialized a player, set a new player component, and then initialized a new player. Being able to override the player component is itself a pretty new capability. It used to be explicitly disallowed, I believe.

I think the thought is that if someone is iterating through all the players, which hooks in video.js do, it's possible that different player components may or may not have the same functionality. So, you may break if you're expecting a method, but it isn't available on all instances.

[gkatsev]

Can I ask what you're using the custom player for? And what issue did you encounter that you hit this block? Maybe there are alternatives.

[victordidenko]

We are making our own interface for the player, using video.js component system. Partly it is base components, partly — our own components, which extends base components. And they are packed in a completely different layout, than base one.

We used to draw an external UI layer on top of the video.js, which hooks on actions and events from the player, but when the player itself gives us such a rich extending capabilities, we decided that it might be easier to make an UI using "native" video.js components.

And as long as Player itself is a component, and when I saw, that video.js gets a Player class from the DI upon creating player — we made a subclass for it as well. And we are replacing base Player class with our subclass.

For now all it does — adds a custom CSS classes from className option, because base player doesn't do that (classes are applied only to children when children are created, but not to Player itself).

And then I've added possibility to use different classes, if needed, effectively replacing Player component in DI before creating player. This is the moment when I hit this limitation from the topic start.

Actually we don't use (and don't plan to use) different classes for players, just this one single subclass I mentioned earlier. And also we don't have pages in the app, where two or more players coexists. So, this is not a big deal and I can remove this feature easily from our codebase, so this question is basically was out of curiosity. Because I tried to create several players, each one based on a different class, on the single page, during the tests, and didn't see an issue.

But I understand precaution now, from your explanation about methods, which might be absent or with different behavior.