feat: add mediator middleware type for play() #4868

ldayananda · 2018-01-09T15:43:56Z

Description

This will allow middleware to interact with calls to play() from the tech. This will require a method of indicating to middleware previously run that a middleware down the chain has terminated or stopped execution.

Specific Changes proposed

Adds middleware mediator method that runs middleware from the player to the tech and a second time back up to the player. This category was created because play is both a setter(changes the playback state) and a getter(gets a native play promise if available). This also has the ability to tell whether a middleware has terminated before reaching the tech.
Adds a middleware.TERMINATOR sentinel value that is available on the videojs object
Adds play to the allowedMediators
Adds paused to the allowedGetters
Adds a sandbox example of a play mediator middleware

Checklist

ldayananda · 2018-01-09T19:43:04Z

src/js/player.js

@@ -1632,6 +1632,9 @@ class Player extends Component {
    this.ready(function() {
      if (method in middleware.allowedSetters) {
        return middleware.set(this.middleware_, this.tech_, method, arg);
+
+      } else if (method in middleware.allowedMediators) {


player.play() seems to be done via techGet for the html5 tech, but it's conceivable that a future mediator will use techCall

maybe we should just make a techCallAndGet for stuff like play?

That might make sense; I don't feel strongly about it one way or another.

Let's get everything else ready and then come back to this.

eh, I think we can just leave it like this.

Yeah, doesn't feel like there's a pressing need to change this yet

gkatsev · 2018-01-10T18:51:08Z

src/js/tech/middleware.js

@@ -2,6 +2,8 @@ import { assign } from '../utils/obj.js';

 const middlewares = {};

+export const TERMINATOR = 'TERMINATOR';


we should make this be an object instance rather than a string since objects only equal themselves. Then the string will also be a valid value.

what do you mean by "the string will also be a valid value"?

Did you mean change this to export const TERMINATOR = () => { return 'TERMINATOR'; } and then here:

https://github.com/ldayananda/video.js/blob/52011c3bc175897e735ec5018159de222db0980c/src/js/video.js#L752

change it to be videojs.TERMINATOR = TERMINATOR() ?

nope, literally just TERMINATOR = {}

Oh, you meant "the string will also be a valid value" to mean that an allowed method could return the string 'TERMINATOR'! 👍 will do

misteroneill · 2018-01-16T17:25:02Z

src/js/video.js

@@ -749,5 +749,7 @@ videojs.dom = Dom;
 */
 videojs.url = Url;

+videojs.TERMINATOR = TERMINATOR;


If we have access to Object.defineProperty we should probably use it here to prevent overwriting this value. Others may think that's overkill, though.

Seems reasonable, I'll do that

ldayananda · 2018-01-16T23:04:55Z

src/js/video.js

@@ -749,7 +749,9 @@ videojs.dom = Dom;
 */
 videojs.url = Url;

-videojs.TERMINATOR = TERMINATOR;
+Object.defineProperty(videojs, 'TERMINATOR', {


@misteroneill does this seem right?

gkatsev · 2018-01-17T16:15:51Z

src/js/video.js

@@ -749,5 +749,9 @@ videojs.dom = Dom;
 */
 videojs.url = Url;

+Object.defineProperty(videojs, 'TERMINATOR', {


this will break in IE8 :)

Also, I think it's reasonable to make it enumerable and it might be nice to make writeable false be explicit.

Is there an example somewhere else in the project of how to do this for IE8 as well? I tried to find one earlier and came up empty.

🆗 explicit writeable: false and enumerable: true sound good to me

something like

if (Object.defineProperty && !browser.IS_IE8) { Object.defineProperty(...); } else { videojs.TERMINATOR = TERMINATOR; }

gkatsev · 2018-01-17T16:16:18Z

src/js/video.js

@@ -749,5 +749,9 @@ videojs.dom = Dom;
 */
 videojs.url = Url;

+Object.defineProperty(videojs, 'TERMINATOR', {


should this be directly on videojs? Maybe namespaced videojs.middleware.TERMINATOR?

I like the idea of it being namespaced to videojs.middleware.TERMINATOR

gkatsev · 2018-01-17T16:36:16Z

src/js/tech/middleware.js

 };

 export const allowedSetters = {
  setCurrentTime: 1
 };

+export const allowedMediators = {
+  play: 1


are we adding pause here as well?

Oh, I should. Thanks for catching that

gkatsev · 2018-01-17T16:37:17Z

src/js/tech/middleware.js

+  for (let i = mws.length - 1; i >= 0; i--) {
+    const mw = mws[i];
+
+    mw[method](value, terminated);


can we swap the argument order so it's similar to a node-style callback?

Good idea, we can do that.

gkatsev · 2018-01-17T16:41:01Z

src/js/player.js

@@ -1632,6 +1632,9 @@ class Player extends Component {
    this.ready(function() {
      if (method in middleware.allowedSetters) {
        return middleware.set(this.middleware_, this.tech_, method, arg);
+
+      } else if (method in middleware.allowedMediators) {


Let's get everything else ready and then come back to this.

gkatsev · 2018-01-17T16:42:42Z

src/js/big-play-button.js

@@ -59,7 +59,7 @@ class BigPlayButton extends Button {

    const playFocus = () => playToggle.focus();

-    if (isPromise(playPromise)) {
+    if (playPromise && isPromise(playPromise)) {


isPromise should be doing a null check as well, no, this shouldn't be strictly necessary.

I did notice that isPromise was checking for undefined specifically. I thought that was for a specific reason, but maybe I was wrong?

if play() is called and no promise was returned, we end up with undefined. Though, we'll never go into this if statement if playPromise doesn't duck-type into a promise.
I'm ok with leaving it if you want, but it shouldn't be necessary.

Ah, while I was testing, if the play was terminated then I saw an error coming from this if statement. Perhaps the right way is to return something other than null in the cases where a middleware terminates.

Ah, I see. Maybe we should just loosen the check in isPromise

I'm fine with going either way - I'll check for null in isPromise for now

gkatsev · 2018-01-17T23:32:11Z

src/js/video.js

@@ -751,8 +751,8 @@ videojs.url = Url;

 // Object.defineProperty is not available in IE8
 if (!browser.IS_IE8 && Object.defineProperty) {
-  Object.defineProperty(videojs, 'middleware.TERMINATOR', {
-    value: TERMINATOR,
+  Object.defineProperty(videojs, 'middleware', {


you'd want defineProperty each step because now you could overwrite videojs.middleware.TERMINATOR = false

ldayananda · 2018-01-23T19:50:27Z

docs/guides/middleware.md

@@ -0,0 +1,116 @@
+# Middleware


This will have to be rebased once the current middleware guide is merged

gkatsev · 2018-01-25T16:32:01Z

Looking at this version of the middleware guide (https://deploy-preview-4868--videojs-docs.netlify.com/tutorial-middleware.html), I realized we forgot to document the "special" setTech method. Also, looks like we say we have to return an object with these methods but an instance of a class or prototype or w/e will work too. But we can do this as part of a separate PR, probably.
Also, the package-lock has conflicts, yay! (sorry)

gkatsev · 2018-01-25T16:34:48Z