fix: always return a promise from play, if supported (videojs#5227)

brandonocasey · gkatsev · commit 58405fdf7811 · 2018-08-10T16:27:05.000-04:00
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()`.
diff --git a/src/js/player.js b/src/js/player.js
@@ -1972,12 +1972,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_();
+  }
+
+  /**
+   * 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_) {
@@ -1997,12 +2019,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
@@ -2014,11 +2037,12 @@ class Player extends Component {
 
       this.playOnLoadstart_ = () => {
         this.playOnLoadstart_ = null;
-        silencePromise(this.play());
+        callback(this.play());
       };
 
       this.one('loadstart', this.playOnLoadstart_);
     }
+
   }
 
   /**
diff --git a/test/unit/player.test.js b/test/unit/player.test.js
@@ -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) => {
+    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) {
