Add a vjs.Player externs file for fullscreen functionality #853
Conversation
availble -> available
Use string literals to lookup fullscreen-related methods on the player object in the fullscreenToggle component. Otherwise, closure compiler replaces them with minified method names and makes it impossible to supply a simpler "player" object with customized fullscreen logic for the fullscreenToggle to interact with.
Instead of using string literals, create an externs file for the player and use that to ensure fullscreen functionality is never minified.
|
Nice, so that worked well enough. I like that a lot better than exports. |
|
Yeah, it certainly removes a lot of goofiness from the implementation of FullscreenToggle. |
|
I think a similar change may also need to be applied to the tech level. https://github.com/videojs/video.js/blob/master/src/js/player.js#L843 tries to call It may be enough to change this line to call |
|
@christophercurrie, I don't see that error when I go to full screen in IE. Could you open a new issue and provide more details? |
| @@ -103,3 +103,33 @@ test('videojs.players should be availble after minification', function() { | |||
|
|
|||
| player.dispose(); | |||
| }); | |||
|
|
|||
| test('fullscreenToggle does not depend on minified player methods', function(){ | |||
There was a problem hiding this comment.
Everything looks great. I don't love this test. It's good for proving that the new way works, but it feels like "don't use these things that don't exist anymore", and you could end up creating one of these tests for every use of a player method by every component. I can't think of a better way to cover this though except maybe to state that we're moving to use externs instead of exports permanently now, so this shouldn't be an issue unless we change our minds on that?
Also, you might be able to simplify this to:
ok(player.requestFullScreen === player['requestFullScreen']);
player.requestFullScreen = false;
ok(player.requestFullScreen === player['requestFullScreen']);
Basically making sure that the non-string version is never minified, because then the fullscreen toggle wouldn't have a minified version available to it. At least I think that would cover it.
|
Filed my other issue as #864, thanks! |
|
Was adding isFullScreen to the externs actually needed somewhere or were you just being complete? I'm realizing it doesn't currently match the rest of the API, using functions instead of properties, so we may want to change that. |
|
It was, otherwise the onClick handler would always think the player wasn't in fullscreen mode. I agree it looks out of place, though. |
|
One difference with externs and exports is that closure compiler might still remove a method that's externed if it's not used anywhere in the code. I found this with the ended() function, which actually wasn't even exported before, but it I added a test for it and found that it was missing. Since it's not used anywhere in the code except to create it, closure compiler was removing it all together if it was only externed. I also have an update coming that changes isFullScreen to be function, in case you're using it anywhere in a plugin. |
Use an externs file for fullscreen-related functionality on the player object. Otherwise, closure compiler replaces them with minified method names and makes it impossible to supply a simpler "player" object with customized fullscreen logic for the fullscreenToggle to interact with.