Advanced Plugins for 6.0

[misteroneill]

Advanced/Class-based Plugins for 6.0

As part of 6.0, we want to make some improvements to plugins while still supporting the current style of plugins for the most part. There are some minor backward-incompatibilities, but nothing that's likely to break existing plugins.

Though this is technically a breaking change, the old stuff should still work!

Goals

These are the broad feature goals of this effort. The overarching approach will be to maintain simplicity throughout the implementation, avoiding assumptions and opinions whenever possible. For now, we are providing a foundation not a framework, but something both we and our users can build on easily.

• Maintain support for "basic" plugins (i.e. a function that is added to Player.prototype).
• Add a foundation for "enhanced" plugins with:
  • Support for the basic lifecycle of initialization and disposal.
  • Support for per-player state management.
• For both types of plugins, add the following:
  • Support for querying per-player plugin activation state.
  • Support for a conventional VERSION property.

Specific Changes

Changes that are backward-incompatible are marked with [BI].

• Add videojs.registerPlugin and videojs.registerPlugins and deprecate videojs.plugin.
  • [BI] Registering a plugin with a name that already exists or conflicts with a Player.prototype method will now throw an error.
• Add videojs.getPlugin, videojs.getPlugins, and videojs.getPluginVersion.
• Add Player#usingPlugin to detect if a plugin has been initialized on a player.
• Create Plugin base class for enhanced plugin functionality.
  • This includes static methods for de-registering plugins, but those are not globally exposed and mostly used for testing.
• Introduce semi-formal concept of mixins, which are functions that mutate an object in some way to add functionality:
  • Create mixins/evented, which provides objects with methods from Events (on, one, off, and trigger). This uses an eventBusEl_ as an event bus because the Events module doesn't play very nicely with non-element objects.
  • Create mixins/stateful, which provides objects with a React-like state property and setState method - as well as triggering a "statechanged" event if the object has a trigger method.

Requirements Checklist

• Feature implemented / Bug fixed
• If necessary, more likely in a feature request than a bug fix
  • Unit Tests updated or fixed
  • Docs/guides updated
  • Example created (starter template on JSBin)
• Reviewed by Two Core Contributors

[gkatsev]

should also check Player.prototype[name]

[dmlap]

I kind of like player.plugin and player.foo where foo is a plugin. Why should we deprecate these?

[misteroneill]

@dmlap You mean videojs.plugin? Assuming that's the case, I felt like it was inconsistent with similar methods (registerComponent and registerTech).

As for player.foo, I'm not married to the new implementation, but the intent was to minimize (or eliminate) the surface area for conflicts between plugins and Player.prototype methods. With this new scheme, you could have a plugin called play or pause; the only limitation is that it can't share the same name as another plugin.

That said, what about an alternative where the plugin was still exposed as player.foo (for the initialization function) and then the other methods were attached to that on init, e.g. player.foo.dispose(). That's a pattern we've used in a few plugins.

[gkatsev]

Fwiw, I don't think having a plugin called play is something we should support as long as it gets attached to the player prototype.

[misteroneill]

For sure. That's more of a long-term design goal, since we're still mutating the Player.prototype.

[gkatsev]

I think 6.0 is a good time to drop support for that.

[heff]

Naming it registerPlugin makes sense to me. 👍

The biggest question I see people run into with complex plugins is storing state/properties for just the plugin, and it'd be great if we could answer that question with this update. At the same time I would love to not lose the simplicity of the current API and the ability to create first-class functions on the player. e.g.

// complex
videojs.registerPlugin('fooState', {
  init: function(options){
    this.foo = options.foo;

    // this.player_ automatically added to instance object 
    this.player_.play();
  },
  isFoo: function(){
    return this.foo;
  }
});

player.fooState({ foo: true });
player.fooState.isFoo();

// simple
videojs.registerPlugin('jumpTo10', function(){
    this.currentTime(10);
});

// Allowing plugin authors to do this is powerful, e.g. jQuery
player.jumpTo10();

That feels perty to me.

[misteroneill]

@heff Thanks for the feedback. It seems we're on the same page here because what I've implemented is very similar (I think) what you described!

I can see keeping the jQuery-style plugins - I'm happy to un-deprecate that. And modifying the API to be more similar to what we have now as both you and @dmlap brought up.

This proposal also includes a concept of state for each plugin/player combination. Which can be understood as behaving as a very lightweight model/view implementation. It works kind of like this currently:

videojs.registerPlugin('foo', {
  init: function(options) {
    this.foo_ = options.foo;
    this.player_; // this is the player.
    this.state_; // this is the state.
  },
  isFoo: function() {
    return this.foo_;
  }
});

player.plugin('foo').init({foo: true});
player.plugin('foo').isFoo(); // true

player.plugin('foo').on('statechange', function() {
  alert('foo changed to ' + this.plugin('foo').isFoo() + '!');
  // Maybe do some DOM manipulation or something here?
});

player.plugin('foo').state({foo: false}); // "foo changed!"

That's without making any changes to what's in the PR at the moment. Like I said, the player.plugin('foo') calls will likely be converted to player.foo.

[misteroneill]

I've been tinkering with replacing player.plugin('foo') with player.foo. And it's definitely do-able, but I worry about some of the unintuitive corner cases.

Particularly around the this context. If we try to stick closer to the current API for a plugin, I feel like this is the expectation:

player.foo(); // this === player
player.foo.isFoo(); // this === player.foo

That's all well and good, but when I create a plugin like this:

videojs.registerPlugin('foo', {
  init: function() {},
  isFoo: function() {}
});

My natural inclination would be that inside any of the functions I passed in for a "complex" plugin, this would be the plugin object.

So, there's a mismatch of expectations there. Either we make the player.foo() call bound to itself for consistency with the expectations from registering the plugin or we make player.foo() bound to the player and break that expectation.

[misteroneill]

In general, my philosophy is to avoid tricky things like this. As it stands, my new implementation does a lot of confusing things, like creating a wrapper function on the Player.prototype then replacing that wrapper function with another wrapper function on a player itself. A wrapper which is, of course, bound to itself! 😆

// Create a Player.prototype-level plugin wrapper that only gets called
// once per player.
plugins[name] = Player.prototype[name] = function(...firstArgs) {

  // Replace this function with a new player-specific plugin wrapper.
  const wrapper = function(...args) {
    if (this.active_) {
      this.teardown();
    }

    this.trigger('beforesetup');
    plugin.setup(...args);
    this.trigger('setup');
  };

  // Wrapper is bound to itself to preserve expectations.
  this[name] = Fn.bind(wrapper, wrapper);

[misteroneill]

I'm tinkering with API ideas in a JSBin.

[heff]

I think it's fine to distinguish simple vs complex plugins, where the context of a simple function is the player and the context of complex plugin functions is the plugin. That's not a hard concept to learn, and the APIs are different enough.

// what happens here? should we magically dispose? or throw? or fail silently?
//    we could dispose OUR stuff but not call `dispose()`
//    we could provide an option to disposeOnSetup?
player.enhancedFoo();
player.enhancedFoo();

Ideally I think a console error logged. If people want to reset a plugin they need to call dispose first. If we try to magically support that I think it would actually lead to more confusion.

[mmcc]

As we discussed quasi-IRL, I think it makes a lot of sense to follow the React API (particularly ES6) for plugins. I think we were circling around something like this:

class EnhancedPlugin extends Videojs.Plugin {
  constructor (opts) {
    super(opts);

    // default state
    this.state = {
      bar: 'baz'
    };
  }

  dispose () { }

  bar () {
    return this.state.bar;
  }

  doSomethingFun () {
    this.setState({bar: 'we are having fun now'});
  }
}

[misteroneill]

Yep, that's very close to what I've got in mind. I'm going to be updating this PR this week with the changes we discussed.

[misteroneill]

At this point, Component has been reimplemented as an evented object - I wanted to make sure it would work - but that change is going to be rolled back to keep the scope of this a bit more sane.

[misteroneill]

This change is repeated in a few places. The previous test assumed that the GUID was 5 digits long, which isn't a reliable assertion. Implementing the evented mixin in Component caused each of these assertions to fail.

[misteroneill]

And the Component changes are rolled back! I'll open a separate PR for those changes.

EDIT: Here is a comparison of the branch implementing the evented mixin for components.

[dmlap]

This is a big PR and it looks like some non-plugin related code snuck in. It would be nice to have the plugin-related changes in an isolated PR.

[dmlap]

videojs.getPlugin('Plugin') looks a little weird to me. How would you feel about plopping the plugin base class at videojs.Plugin?

[misteroneill]

I agree that it's a bit weird, but... so is videojs.getComponent('Component'), no? I think it's more important that the API be consistent/predictable.

[dmlap]

You might want to collapse this and the previous section since they're identical to the basic plugin examples above.

[misteroneill]

I think that makes sense. I'll reshuffle this doc a bit to eliminate some of the duplication.

[dmlap]

This doesn't seem to add much for end users that they couldn't achieve by repeating registerPlugin() a couple times in a row.

[misteroneill]

Agreed. Will remove this and deregisterPlugins, which was really only added for my own convenience.

[dmlap]

The changes to this file look nice but it seems unrelated to this PR.

[misteroneill]

Yes, it was previously rebased on another branch while I waited for that to be merged. If my other PRs for removing lodash and object.assign are merged, this will be rebased again and the changes should shrink further.

[thijstriemstra]

@misteroneill is this test replicated elsewhere since it's removed here? Because it was a pita to add it I remember. I have a hard time finding it or something similar in this big changeset.

[misteroneill]

It looks like I missed the test for this feature. Nice catch, thanks!

[misteroneill]

This is now tested in player.test.js, under the "options: plugins" test. I've changed so it throws now instead of just logging. Failing loud in this case seems like a good idea.

[thijstriemstra]

Looks good, thanks!

[brandonocasey]

LGTM

[theartoftraining99]

Virtualisation