-
Notifications
You must be signed in to change notification settings - Fork 795
Fix tech.play() throwing unresolved promise errors on Chrome #1338
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, just one minor comment
src/master-playlist-controller.js
Outdated
|
||
// Catch/silence error when a pause interrupts a play request | ||
// on browsers which return a promise | ||
if (playPromise !== undefined && typeof playPromise.then === 'function') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the linter might warn to use typeof playPromise !== 'undefined'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gesinger cool! I'll update later today.
src/master-playlist-controller.js
Outdated
|
||
// Catch/silence error when a pause interrupts a play request | ||
// on browsers which return a promise | ||
if (playPromise !== undefined && typeof playPromise.then !== 'undefined') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update @grobolom ! Though in this case the typeof comparing to a function is OK (and preferred), but the playPromise !== undefined
should probably be typeof playPromise !== 'undefined'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aaaah. I see. My bad, I'll fix that up!
@gesinger all fixed up 👍 |
@gesinger hey man, just wondering - is there an approximate timeline for when PRs get merged in these repositories? I understand it's not a high-traffic area, but it would be cool for to try this out in our code (and avoid some errors). No rush though, just asking :) |
Hey @grobolom , sorry about the delay. No set timeline, we were just focused on some other issues. That said, this is all set to go except we do a quick round of manual testing before merging it in. We'll try to get around to it today if we can. |
@gesinger no sweat - thanks for adding this in! We really appreciate it :) |
Description
This is an enhancement that fixes errors being thrown by videojs-contrib-hls on Chrome. While the error does not impact player functionality, it pollutes the console log, which may result in confusion or time wasted during manual/automated testing. The error is caused by the media
.play()
event returning a Promise in Chrome, which throws the errorThe play() request was interrupted by a call to pause()
.The fix is mirrored from a similar fix in the video.js core repo, which simply wraps the return value of
.play()
in a promise if necessary.Specific Changes proposed
This fix proposes to wrap the
this.tech_.play()
call inhandleSourceOpen_()
in themaster-playlist-controller
with a.then()
, so that Chrome does not show a Promise-based error.Requirements Checklist