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.

Sign up for GitHub

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

Warn if options / ready function are passed to a previously created player instance #1730

Closed
wants to merge 1 commit into from
Closed

Warn if options / ready function are passed to a previously created player instance #1730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/guides/setup.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ Step 2: Add an HTML5 video tag to your page.
--------------------------------------------
With Video.js you just use an HTML5 video tag to embed a video. Video.js will then read the tag and make it work in all browsers, not just ones that support HTML5 video. Beyond the basic markup, Video.js needs a few extra pieces.

> Note: The `data-setup` attribute described here should not be used if you use the alternative setup described in the next section.

1. The 'data-setup' Attribute tells Video.js to automatically set up the video when the page is ready, and read any options (in JSON format) from the attribute (see [options](options.md)). There are other methods for initializing the player, but this is the easiest.

2. The 'id' Attribute: Should be used and unique for every video on the same page.
Expand Down
9 changes: 9 additions & 0 deletions src/js/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,15 @@ var vjs = function(id, options, ready){

// If a player instance has already been created for this ID return it.
if (vjs.players[id]) {

// If options or ready funtion are passed, warn
if (options) {
vjs.log.warn ('Player "' + id + '" is already initialised. Options will not be applied.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we couldn't still handle options and update the values in this case. Though handling options like children after init would get super complicated, so probably not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deep merging is hard. But otherwise, yes, it could happen.

}
if (ready) {
vjs.log.warn ('Player "' + id + '" is already initialised. Ready function will not be executed.');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I think we can actually still use the player's ready function.

player.ready(ready);

The ready function has a feature where if ready as already fired, it will immediately execute the function passed to it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to make that be asynchronous rather than calling it sync in the case of ready already happening. Otherwise, it's good.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gkatsev yeah, that's a separate issue from this specifically. #1667

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, #1667 is specifically talking about triggerReady. We should, however, add just regular ready as part of #1667.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok cool, feel free to add that to the other issue.

}

return vjs.players[id];

// Otherwise get element for ID
Expand Down