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.

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

Fix YouTube onReady bugs #21

Merged
merged 1 commit into from
Jan 3, 2016
Merged

Fix YouTube onReady bugs #21

merged 1 commit into from
Jan 3, 2016

Conversation

cookpete
Copy link
Owner

@cookpete cookpete commented Jan 2, 2016

Attempts to fix #20

@Fauntleroy I'm hoping this does the job? Could you confirm?

@Fauntleroy
Copy link
Contributor

Doesn't look like this fixes it... but I've got plenty of free time and a reproducible test case. I can go ahead and take care of this myself in a separate pull req if it's all the same to you

@Fauntleroy
Copy link
Contributor

I spent quite a while working on reproducing this bug, making a test for it, and resolving it—but I could only get one of those complete. Here are the steps I used to replicate my issue:

  • The player must be initialized with playing set to true and url set to null
  • The YouTube player must be set to preload in the youtubeConfig
  • Immediately after the player initializes, the url must be set to another YouTube URL (I did this within the test app's componentDidMount call)

This will cause the player to attempt to prime the YouTube player with the empty video, thus calling load, which ultimately calls new YT.player. Immediately after that the url is updated, which causes load to be called again, calling new YT.player and overwriting its original value. Eventually the first player's onReady fires, but by that time the second player (which is still initializing) is the value of this.player. Thus we try to call this.player.playVideo() (and other methods) when one of these players has initialized, but the other hasn't.

My idea for fixing this would be to add a flag for keeping track of whether or not we've started initializing a YouTube player yet. Using this flag, you could check to see if a player is pending during a load call, then act accordingly. If no player is initializing, create a new YouTube player. If a player is initializing, add the video ID to a queue and fire loadVideo or playVideo when onReady fires.

@cookpete
Copy link
Owner Author

cookpete commented Jan 3, 2016

Eventually the first player's onReady fires

This PR should prevent that first onReady from firing.

@cookpete
Copy link
Owner Author

cookpete commented Jan 3, 2016

Updated with a new approach. @Fauntleroy any better?

@cookpete
Copy link
Owner Author

cookpete commented Jan 3, 2016

Argh, still not good enough. Pausing/playing is now screwed up.

@cookpete
Copy link
Owner Author

cookpete commented Jan 3, 2016

Ah, never mind, I missed a ! here which caused problems. Oops.. I've fixed up master and rebased this branch.

@Fauntleroy
Copy link
Contributor

This seems to be functioning as expected in both the react-player example and my application. I'm going to look over your code and see if there are any hidden gotchas before I sign off on this completely, though...

@Fauntleroy
Copy link
Contributor

Having trouble with this in production. Let me double check my work first

@Fauntleroy
Copy link
Contributor

Looks like the issue was probably caused by caching on my production server. Everything looks good to me here, though we should really start investing in tests as we move forward.

@cookpete
Copy link
Owner Author

cookpete commented Jan 3, 2016

we should really start investing in tests as we move forward.

Absolutely. Shallow rendering unit tests only get us so far. I'll open an issue.

I'll merge this and publish a patch release.

cookpete added a commit that referenced this pull request Jan 3, 2016
@cookpete cookpete merged commit 84d832d into master Jan 3, 2016
@cookpete cookpete deleted the fix-youtube-ready branch January 3, 2016 16:40
@mgw-sbex mgw-sbex mentioned this pull request Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Better handling of YouTube methods before player is ready
2 participants