-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
feat: always return a promise from play, if supported #5227
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1873,12 +1873,34 @@ class Player extends Component { | |
* Attempt to begin playback at the first opportunity. | ||
* | ||
* @return {Promise|undefined} | ||
* Returns a `Promise` only if the browser returns one and the player | ||
* is ready to begin playback. For some browsers and all non-ready | ||
* situations, this will return `undefined`. | ||
* Returns a promise if the browser supports Promises (or one | ||
* was passed in as an option). This promise will be resolved on | ||
* the return value of play. If this is undefined it will fulfill the | ||
* promise chain otherwise the promise chain will be fulfilled when | ||
* the promise from play is fulfilled. | ||
*/ | ||
play() { | ||
const promise = this.options_.Promise || window.Promise; | ||
|
||
if (promise) { | ||
return new Promise((resolve) => { | ||
this.play_(resolve); | ||
}); | ||
} | ||
|
||
return this.play_(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ie11 does not support promises, so that should be the only platform this happens on. |
||
} | ||
|
||
/** | ||
* The actual logic for play, takes a callback that will be resolved on the | ||
* return value of play. This allows us to resolve to the play promise if there | ||
* is one on modern browsers. | ||
* | ||
* @private | ||
* @param {Function} [callback] | ||
* The callback that should be called when the techs play is actually called | ||
*/ | ||
play_(callback = silencePromise) { | ||
// If this is called while we have a play queued up on a loadstart, remove | ||
// that listener to avoid getting in a potentially bad state. | ||
if (this.playOnLoadstart_) { | ||
|
@@ -1898,12 +1920,13 @@ class Player extends Component { | |
this.playWaitingForReady_ = true; | ||
this.ready(() => { | ||
this.playWaitingForReady_ = false; | ||
silencePromise(this.play()); | ||
callback(this.play()); | ||
}); | ||
|
||
// If the player/tech is ready and we have a source, we can attempt playback. | ||
} else if (!this.changingSrc_ && (this.src() || this.currentSrc())) { | ||
return this.techGet_('play'); | ||
callback(this.techGet_('play')); | ||
return; | ||
|
||
// If the tech is ready, but we do not have a source, we'll need to wait | ||
// for both the `ready` and a `loadstart` when the source is finally | ||
|
@@ -1915,11 +1938,12 @@ class Player extends Component { | |
|
||
this.playOnLoadstart_ = () => { | ||
this.playOnLoadstart_ = null; | ||
silencePromise(this.play()); | ||
callback(this.play()); | ||
}; | ||
|
||
this.one('loadstart', this.playOnLoadstart_); | ||
} | ||
|
||
} | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1136,6 +1136,7 @@ if (window.Promise) { | |
} | ||
|
||
QUnit.test('play promise should resolve to native value if returned', function(assert) { | ||
const done = assert.async(); | ||
const player = TestHelpers.makePlayer({}); | ||
|
||
player.src({ | ||
|
@@ -1148,7 +1149,18 @@ QUnit.test('play promise should resolve to native value if returned', function(a | |
player.tech_.play = () => 'foo'; | ||
const p = player.play(); | ||
|
||
assert.equal(p, 'foo', 'play returns foo'); | ||
const finish = (v) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. now that play will always return a promise, if it is supported, we have to use |
||
assert.equal(v, 'foo', 'play returns foo'); | ||
done(); | ||
}; | ||
|
||
if (typeof p === 'string') { | ||
finish(p); | ||
} else { | ||
p.then((v) => { | ||
finish(v); | ||
}); | ||
} | ||
}); | ||
|
||
QUnit.test('should throw on startup no techs are specified', function(assert) { | ||
|
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 wonder if it's possible to have
play_
return a promise itself or use a promise we give it. Maybe a bit tricky since ourplay_
method has some event listeners.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.
whats really tricky is that IE 11 does not have promise support, so we can't technically return a promise all the time.
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.
yeah, IE11 aside, I mean.
We can require a polyfill for the best behavior here, as long as we don't fail if one doesn't provided.