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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@mister-ben
Contributor

mister-ben commented Dec 9, 2014

There are questions fairly regularly on Stack Overflow that come down to confusion over using both data-setup and passing options and/or a ready function to videojs().

This change logs a message with vjs.log if options or a ready function are passed as arguments, if a previously created player is returned. It also adds note in the setup doc.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Dec 9, 2014

Member

Nice! Handling this is much needed.

Member

heff commented Dec 9, 2014

Nice! Handling this is much needed.

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

This comment has been minimized.

@heff

heff Dec 9, 2014

Member

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.

@heff

heff Dec 9, 2014

Member

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.

This comment has been minimized.

@gkatsev

gkatsev Dec 9, 2014

Member

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

@gkatsev

gkatsev Dec 9, 2014

Member

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

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

This comment has been minimized.

@heff

heff Dec 9, 2014

Member

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.

@heff

heff Dec 9, 2014

Member

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.

This comment has been minimized.

@gkatsev

gkatsev Dec 9, 2014

Member

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.

@gkatsev

gkatsev Dec 9, 2014

Member

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.

This comment has been minimized.

@heff

heff Dec 9, 2014

Member

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

@heff

heff Dec 9, 2014

Member

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

This comment has been minimized.

@gkatsev

gkatsev Dec 9, 2014

Member

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

@gkatsev

gkatsev Dec 9, 2014

Member

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

This comment has been minimized.

@heff

heff Dec 9, 2014

Member

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

@heff

heff Dec 9, 2014

Member

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

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Dec 16, 2014

Member

@mister-ben can I get you to make the player.ready(ready); change mentioned?

Member

heff commented Dec 16, 2014

@mister-ben can I get you to make the player.ready(ready); change mentioned?

@heff heff closed this in f0d500a Dec 22, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Dec 22, 2014

Member

Merged into master. Thanks Ben!

Member

heff commented Dec 22, 2014

Merged into master. Thanks Ben!

@mister-ben

This comment has been minimized.

Show comment
Hide comment
@mister-ben

mister-ben Dec 23, 2014

Contributor

Thanks! I've been away with no internet for a couple of weeks, so hadn't been able to get around to that last change.

Contributor

mister-ben commented Dec 23, 2014

Thanks! I've been away with no internet for a couple of weeks, so hadn't been able to get around to that last change.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Dec 23, 2014

Member

No problem. Thanks for the help!

Member

heff commented Dec 23, 2014

No problem. Thanks for the help!

@mister-ben mister-ben deleted the mister-ben:initialised-player-warning branch Dec 30, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment