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

brandonocasey · 2018-06-04T20:11:58Z

Description

Always return a Promise from play when we have a Promise object.

brandonocasey · 2018-06-04T21:08:50Z

test/unit/player.test.js

@@ -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) => {


now that play will always return a promise, if it is supported, we have to use then to check the resolved value.

brandonocasey · 2018-06-04T21:09:31Z

src/js/player.js

+      });
+    }
+
+    return this.play_();


ie11 does not support promises, so that should be the only platform this happens on.

gkatsev

This is probably a good first step, we should make sure that this is set up to adding rejecting the promise properly as well. We might want to do that in a separate PR though.

gkatsev · 2018-06-15T19:54:34Z

src/js/player.js


+    if (promise) {
+      return new Promise((resolve) => {


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 our play_ method has some event listeners.

whats really tricky is that IE 11 does not have promise support, so we can't technically return a promise all the time.

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.

brandonocasey · 2018-06-18T18:35:00Z

Two questions for you @gkatsev.

By " sure that this is set up to adding rejecting the promise properly as well" Do you mean that we should make sure to silence the play promise? Or do you mean that we should just make sure that the promise we return will be rejected on a rejected play promise? If its number two I think we already will do that because we resolve our promise on the play promise. Then that play promise will actually determine if we are rejected/resolved.
Why do we want to return a promise from play_? I added play_ because returning a promise from play was really messy when their wasn't always a PromiseClass to use. We could return a fake promise in those cases which would solve it, but I thought it would be better to not do that, so I opted for play_ to take a callback which will be the resolve function when working with promises, or a noop when there are no promises.

gkatsev · 2018-08-08T18:31:25Z

I guess I never responded here.

The idea is that we want to shim the play promise behavior for when the play() doesn't return a promise natively. Like on IE11. So, any change we do now, hopefully shouldn't be too complex that would require us to rewrite a lot of it as part of that work.
I think that what we have now works well if there is a native play promise and a step towards our goals. Though, maybe we should even check if just window.Promise exists right now because otherwise IE11 will return a promise that never resolves. Or maybe even do an IE11-specific check as well.
Looking at the code more, my suggestion is probably not helpful/doesn't make much sense based on the current implementation. I'm mostly trying to reduce usage of Promise constructor and passing around the inner resolve/reject methods.

brandonocasey · 2018-08-08T18:49:44Z

So if we want to try and shim the play promise behavior on IE 11. Does that mean we want to:

Create a shim Promise that is returned from Player#play() when we do not have a real promise to use.
Check if the tech returns a Promise from it's play method and if it does not return, resolve the Player#play promise on the play event.

gkatsev · 2018-08-08T18:51:47Z

Yeah, more or less. It gets a bit trickier because you'd want to reject the promise if you get a pause before play and stuff like that but essentially if a native play promise isn't provided we want to make our own promise.

gkatsev · 2018-08-08T19:19:40Z

My main worry was that on IE11 if a Promise Polyfill was on the page, we'd end up returning a forever-pending promise, but I think that we'd get a promise that gets resolved once play_ does its work, it would just not be rejected and that's probably good enough for this PR.

gkatsev

I want to take this for a quick spin but otherwise, it's good to go. I'll work on making a full play promise shim afterwards.

brandonocasey · 2018-08-08T21:29:30Z

awesome, also make sure in the next pr to add something here: https://github.com/videojs/video.js/pull/5227/files#diff-3b0266ff1c037b289ec624ab25b0272eR1928

that will check to see if there is a play promise and if not it will fallback to using play/pause events

If Promise is available or if Promise is polyfilled, then we'll return a new Promise from `play()` that will get fulfilled with the value returned from the `play()` method. This means that on IE11, this promise will get resolved when we call `play()` even if play doesn't succeed. We will have a follow-on PR to polyfill this behavior and resolve or reject the promise for browsers like IE11 that don't return a promise from `play()`.

feat: always return a promise from play, if supported

24f2017

brandonocasey mentioned this pull request Jun 4, 2018

feat: manual autoplay #5209

Merged

brandonocasey commented Jun 4, 2018

View reviewed changes

gkatsev mentioned this pull request Jun 6, 2018

VideoJS 7 fails to return the native play promise #5218

Closed

test refactor

9789bf4

brandonocasey force-pushed the feat/always-return-promise branch from ea148de to 9789bf4 Compare June 11, 2018 15:39

gkatsev reviewed Jun 15, 2018

View reviewed changes

use the resolved PromiseClass

1d11cfd

ayamamori mentioned this pull request Jul 3, 2018

fix: autoplay throws 'undefined promise' error on some browsers. #5283

Merged

7 tasks

gkatsev approved these changes Aug 8, 2018

View reviewed changes

gkatsev mentioned this pull request Aug 9, 2018

Player.play() does not return promise #5362

Closed

gkatsev merged commit 58405fd into master Aug 10, 2018

gkatsev deleted the feat/always-return-promise branch August 10, 2018 20:27

PoojaPanchal12 mentioned this pull request Aug 31, 2024

[Snyk] Upgrade video.js from 6.12.0 to 6.13.0 CleverTap/vue-video-player#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

brandonocasey commented Jun 4, 2018 •

edited by gkatsev

Loading

brandonocasey Jun 4, 2018 •

edited

Loading

brandonocasey Jun 4, 2018

gkatsev left a comment

gkatsev Jun 15, 2018

brandonocasey Jun 18, 2018

gkatsev Jun 18, 2018

brandonocasey commented Jun 18, 2018

gkatsev commented Aug 8, 2018

brandonocasey commented Aug 8, 2018

gkatsev commented Aug 8, 2018

gkatsev commented Aug 8, 2018

gkatsev left a comment

brandonocasey commented Aug 8, 2018 •

edited

Loading

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

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

Conversation

brandonocasey commented Jun 4, 2018 • edited by gkatsev Loading

Description

brandonocasey Jun 4, 2018 • edited Loading

Choose a reason for hiding this comment

brandonocasey Jun 4, 2018

Choose a reason for hiding this comment

gkatsev left a comment

Choose a reason for hiding this comment

gkatsev Jun 15, 2018

Choose a reason for hiding this comment

brandonocasey Jun 18, 2018

Choose a reason for hiding this comment

gkatsev Jun 18, 2018

Choose a reason for hiding this comment

brandonocasey commented Jun 18, 2018

gkatsev commented Aug 8, 2018

brandonocasey commented Aug 8, 2018

gkatsev commented Aug 8, 2018

gkatsev commented Aug 8, 2018

gkatsev left a comment

Choose a reason for hiding this comment

brandonocasey commented Aug 8, 2018 • edited Loading

brandonocasey commented Jun 4, 2018 •

edited by gkatsev

Loading

brandonocasey Jun 4, 2018 •

edited

Loading

brandonocasey commented Aug 8, 2018 •

edited

Loading