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

feat: always return a promise from play, if supported #5227

Merged
merged 3 commits into from
Aug 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
36 changes: 30 additions & 6 deletions src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 PromiseClass = this.options_.Promise || window.Promise;

if (PromiseClass) {
return new PromiseClass((resolve) => {
this.play_(resolve);
});
}

return this.play_();
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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_) {
Expand All @@ -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
Expand All @@ -1915,11 +1938,12 @@ class Player extends Component {

this.playOnLoadstart_ = () => {
this.playOnLoadstart_ = null;
silencePromise(this.play());
callback(this.play());
};

this.one('loadstart', this.playOnLoadstart_);
}

}

/**
Expand Down
14 changes: 13 additions & 1 deletion test/unit/player.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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({
Expand All @@ -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) => {
Copy link
Contributor Author

@brandonocasey brandonocasey Jun 4, 2018

Choose a reason for hiding this comment

The 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 then to check the resolved value.

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) {
Expand Down