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: add mediator middleware type for play() #4868

Merged
merged 39 commits into from Jan 30, 2018

Conversation

Projects
None yet
3 participants
@ldayananda
Contributor

ldayananda commented Jan 9, 2018

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

  • Feature implemented / Bug fixed
  • unit tests added
  • documentation added
  • guide added/updated
  • verified in browser
  • example created
  • Reviewed by Two Core Contributors
@@ -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) {

This comment has been minimized.

@ldayananda

ldayananda Jan 9, 2018

Contributor

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

This comment has been minimized.

@gkatsev

gkatsev Jan 10, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 11, 2018

Contributor

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

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 25, 2018

Contributor

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

@@ -2,6 +2,8 @@ import { assign } from '../utils/obj.js';
const middlewares = {};
export const TERMINATOR = 'TERMINATOR';

This comment has been minimized.

@gkatsev

gkatsev Jan 10, 2018

Member

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.

This comment has been minimized.

@ldayananda

ldayananda Jan 10, 2018

Contributor

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

This comment has been minimized.

@ldayananda

ldayananda Jan 11, 2018

Contributor

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() ?

This comment has been minimized.

@gkatsev

gkatsev Jan 11, 2018

Member

nope, literally just TERMINATOR = {}

This comment has been minimized.

@ldayananda

ldayananda Jan 11, 2018

Contributor

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

@ldayananda ldayananda changed the title from [WIP] feat: Add `play()` to middleware to feat: Add `play()` to middleware Jan 13, 2018

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

This comment has been minimized.

@misteroneill

misteroneill Jan 16, 2018

Member

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.

This comment has been minimized.

@ldayananda

ldayananda Jan 16, 2018

Contributor

Seems reasonable, I'll do that

@@ -749,7 +749,9 @@ videojs.dom = Dom;
*/
videojs.url = Url;
videojs.TERMINATOR = TERMINATOR;
Object.defineProperty(videojs, 'TERMINATOR', {

This comment has been minimized.

@ldayananda

ldayananda Jan 16, 2018

Contributor

@misteroneill does this seem right?

@@ -749,5 +749,9 @@ videojs.dom = Dom;
*/
videojs.url = Url;
Object.defineProperty(videojs, 'TERMINATOR', {

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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.

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

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

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

something like

if (Object.defineProperty && !browser.IS_IE8) {
  Object.defineProperty(...);
} else {
  videojs.TERMINATOR = TERMINATOR;
}
@@ -749,5 +749,9 @@ videojs.dom = Dom;
*/
videojs.url = Url;
Object.defineProperty(videojs, 'TERMINATOR', {

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

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

};
export const allowedSetters = {
setCurrentTime: 1
};
export const allowedMediators = {
play: 1

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

are we adding pause here as well?

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

Oh, I should. Thanks for catching that

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

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

Good idea, we can do that.

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

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

@@ -59,7 +59,7 @@ class BigPlayButton extends Button {
const playFocus = () => playToggle.focus();
if (isPromise(playPromise)) {
if (playPromise && isPromise(playPromise)) {

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

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

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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.

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

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.

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 17, 2018

Contributor

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

@@ -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', {

This comment has been minimized.

@gkatsev

gkatsev Jan 17, 2018

Member

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

This comment has been minimized.

@ldayananda

ldayananda Jan 18, 2018

Contributor

Good point

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

This comment has been minimized.

@ldayananda

ldayananda Jan 23, 2018

Contributor

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

@gkatsev

This comment has been minimized.

Member

gkatsev commented Jan 25, 2018

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)

@@ -0,0 +1,184 @@
<!DOCTYPE html>

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

this file should be middleware-play.html.example

margin: 2em 1em;
}
.vjs-progress-control.vjs-control .vjs-play-progress.vjs-slider-bar.terminated {

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

why not look for the terminated class on the player instead?

This comment has been minimized.

@ldayananda

ldayananda Jan 25, 2018

Contributor

What do you mean? I think I needed to be specific to get the right component

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member
.terminated .vjs-process-control .vjs-play-progress

Then below (line 44), you can just do player.addClass('terminated') instead of going through the entire child heirarchy to get the proper element. Or something like that.

console.log('Middleware 1: play has been cancelled prior to this middleware');
}
return value;

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

is this required?

This comment has been minimized.

@ldayananda

ldayananda Jan 25, 2018

Contributor

No, I can remove it

var vid = document.getElementById("vid1");
var player = videojs(vid, {
controlBar: {
children: [

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

is there a reason for these?

This comment has been minimized.

@ldayananda

ldayananda Jan 25, 2018

Contributor

heh, copypasta.

},
// Mediating the results back to the player
play: function(cancelled, value) {
console.log('Middleware 1: play got from tech. What is the value passed?', value);

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

would be nice to have it .then and .catch in here.

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

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

can we put this block immediately after videojs.use so middleware related stuff are grouped together?

* [Using Middleware](#using-middleware)
* [setSource](#setsource)
## Understanding Middleware
Middleware are functions that return an object with methods matching those on the `Tech`. There are currently a limited set of allowed methods that will be understood by middleware. These are: `buffered`, `currentTime`, `setCurrentTime`, `duration`, `seekable` and `played`.
Middleware are functions that return an object with methods matching those on the `Tech`. There are currently a limited set of allowed methods that will be understood by middleware. These are: `buffered`, `currentTime`, `setCurrentTime`, `duration`, `seekable`, `played`, `play` and `pause`.

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

also paused?

This comment has been minimized.

@ldayananda

ldayananda Jan 25, 2018

Contributor

good catch

These allowed methods are split into two categories: `getters` and `setters`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`.
These allowed methods are split into three categories: `getters`, `setters` and `mediators`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`.

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

We're missing a short description of mediators

This comment has been minimized.

@ldayananda

ldayananda Jan 25, 2018

Contributor

Think it's enough to link mediators to the next section "Termination and Mediators"?

This comment has been minimized.

@gkatsev

gkatsev Jan 26, 2018

Member

Probably. Would be nice to get a one-sentence description, though, if we can.

@@ -56,6 +82,24 @@ var myMiddleware = function(player) {
videojs.use('*', myMiddleware);
```
Mediator methods can terminate, by doing the following:

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

would be good to make this a subsection so it can be linked to from above.

Mediators are the third category of allowed methods. These are methods that not only change the state of the Tech, but also return some value back to the Player. Currently, these are `play` and `pause`.
Mediators make a round trip: starting at the `Player`, mediating to the `Tech` and returning the result to the `Player` again. A `call{method}` method must be supplied by the middleware which is used when mediating to the `Tech`. For example: `callPlay`. On the way back to the `Player`, the `{method}` will be called instead, with 2 arguments: `terminated`, a Boolean indicating whether a middleware terminated during the mediation to the tech portion, and `value`, which is the value returned from the `Tech`.

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

I think we should have simple examples here to better illustrate the text.

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

This comment has been minimized.

@gkatsev

gkatsev Jan 25, 2018

Member

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

@ldayananda

This comment has been minimized.

Contributor

ldayananda commented Jan 26, 2018

@gkatsev I think I've now addressed all your comments. I agree that the corrections to the middleware guide can be done in a separate PR. And I removed the package-lock so we should be good now 😄

/**
* Calls a getter on the tech first, through each middleware
* from right to left to the player.
**/

This comment has been minimized.

@gkatsev

gkatsev Jan 26, 2018

Member

nit: only one * for closing :)

These allowed methods are split into two categories: `getters` and `setters`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`.
These allowed methods are split into three categories: `getters`, `setters` and `mediators`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`.

This comment has been minimized.

@gkatsev

gkatsev Jan 26, 2018

Member

Probably. Would be nice to get a one-sentence description, though, if we can.

enumerable: true
});
} else {
videojs.middleware.TERMINATOR = TERMINATOR;

This comment has been minimized.

@gkatsev

gkatsev Jan 26, 2018

Member

this will fail because videojs.middleware isn't an object.
Should be videojs.middleware = {TERMINATOR}

@gkatsev

This comment has been minimized.

Member

gkatsev commented Jan 26, 2018

Oh, the commit message should probably be more like feat: add mediator middleware type for play()

@ldayananda ldayananda changed the title from feat: Add `play()` to middleware to feat: add mediator middleware type for play() Jan 29, 2018

These allowed methods are split into two categories: `getters` and `setters`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`.
These allowed methods are split into three categories: `getters`, `setters` and `mediators`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`. Mediators are called on the `Player` first, run through middleware from left to right, then called on the `Tech` and the result is returned to the `Player` unchanged, while calling the middleware from right to left. For more information, check out the [mediator section](#termination-and-mediators).

This comment has been minimized.

@ldayananda

ldayananda Jan 29, 2018

Contributor

@gkatsev Added a sentence for Mediators, but it's more confusing in my opinion than reading the full section

This comment has been minimized.

@gkatsev

gkatsev Jan 29, 2018

Member

🤔 I think this is ok. Maybe @videojs/core-committers has any other thoughts?

This comment has been minimized.

@misteroneill

misteroneill Jan 29, 2018

Member

I think this paragraph should be split up. I propose breaking it into three distinct sections. Also, made some grammatical improvements.


These allowed methods are split into three categories: getters, setters, and mediators.

Middleware Setters

Setters will be called on the Player first and run through middleware (from left to right) before calling the method, with its arguments, on the Tech.

Middleware Getters

Getters are called on the Tech first and are run though middleware (from right to left) before returning the result to the Player.

Middleware Mediators

Mediators are called on the Player first, run through middleware (from left to right), then called on the Tech. The result is returned to the Player unchanged, while calling the middleware from right to left. For more information on mediators, check out the mediator section.

@gkatsev

LGTM!

These allowed methods are split into two categories: `getters` and `setters`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`.
These allowed methods are split into three categories: `getters`, `setters` and `mediators`. Setters will be called on the `Player` first and run through middleware(from left to right) before calling the method, with its arguments, on the `Tech`. Getters are called on the `Tech` first and are run though middleware(from right to left) before returning the result to the `Player`. Mediators are called on the `Player` first, run through middleware from left to right, then called on the `Tech` and the result is returned to the `Player` unchanged, while calling the middleware from right to left. For more information, check out the [mediator section](#termination-and-mediators).

This comment has been minimized.

@misteroneill

misteroneill Jan 29, 2018

Member

I think this paragraph should be split up. I propose breaking it into three distinct sections. Also, made some grammatical improvements.


These allowed methods are split into three categories: getters, setters, and mediators.

Middleware Setters

Setters will be called on the Player first and run through middleware (from left to right) before calling the method, with its arguments, on the Tech.

Middleware Getters

Getters are called on the Tech first and are run though middleware (from right to left) before returning the result to the Player.

Middleware Mediators

Mediators are called on the Player first, run through middleware (from left to right), then called on the Tech. The result is returned to the Player unchanged, while calling the middleware from right to left. For more information on mediators, check out the mediator section.

@@ -9,7 +9,7 @@
* Whether or not the object is `Promise`-like.
*/
export function isPromise(value) {
return value !== undefined && typeof value.then === 'function';
return value !== undefined && value !== null && typeof value.then === 'function';

This comment has been minimized.

@misteroneill

misteroneill Jan 29, 2018

Member

Mabye we should just do return Boolean(value) && typeof value.then === 'function';

This comment has been minimized.

@ldayananda

ldayananda Jan 29, 2018

Contributor

it's nice to be explicit here too 🤔

if (!browser.IS_IE8 && Object.defineProperty) {
Object.defineProperty(videojs, 'middleware', {
value: {},
writeable: false,

This comment has been minimized.

@misteroneill

misteroneill Jan 29, 2018

Member

I believe writable: false is the default, so this shouldn't be necessary.

This comment has been minimized.

@ldayananda

ldayananda Jan 29, 2018

Contributor

We talked about this elsewhere, but since having writeable: false is very explicit, I plan to keep it 😄

@gkatsev gkatsev merged commit bf3eb45 into videojs:master Jan 30, 2018

4 checks passed

continuous-integration/codeship Build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@ldayananda ldayananda deleted the ldayananda:play-middleware branch Jan 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment