Skip to content
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 'beforepluginsetup' event and named plugin setup events (e.g. 'pluginsetup:foo') #4255

Merged
merged 3 commits into from
May 11, 2017
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 61 additions & 8 deletions src/js/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,26 @@ const markPluginAsActive = (player, name) => {
player[PLUGIN_CACHE_KEY][name] = true;
};

/**
* Triggers a pair of plugin setup events.
*
* @private
* @param {Player} player
* A Video.js player instance.
*
* @param {Plugin~PluginEventHash} hash
* A plugin event hash.
*
* @param {Boolean} [before]
* If true, modifies the events to be "before" events.
Copy link
Contributor

@brandonocasey brandonocasey Apr 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be good to have an example, as this confused me at first. Maybe: 'I.E pluginsetup -> beforepluginsetup'

*/
const triggerSetupEvent = (player, hash, before) => {
const eventName = (before ? 'before' : '') + 'pluginsetup';

player.trigger(eventName, hash);
player.trigger(eventName + ':' + hash.name, hash);
};

/**
* Takes a basic plugin function and returns a wrapper function which marks
* on the player that the plugin has been activated.
Expand All @@ -91,15 +111,20 @@ const markPluginAsActive = (player, name) => {
*/
const createBasicPlugin = function(name, plugin) {
const basicPluginWrapper = function() {

// We trigger the "beforepluginsetup" and "pluginsetup" events on the player
// regardless, but we want the hash to be consistent with the hash provided
// for advanced plugins.
//
// The only potentially counter-intuitive thing here is the `instance` in
// the "pluginsetup" event is the value returned by the `plugin` function.
triggerSetupEvent(this, {name, plugin, instance: null}, true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm up in the air about wether we should pass data along with the event. I understand why we would need to but, it still goes against most of the practices that we have for other events. Anyone else want to chime in here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it's already the case for pluginsetup.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, I guess this is just what we have to do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I do know some plugins do return something from the method, so, providing it is nice.


const instance = plugin.apply(this, arguments);

markPluginAsActive(this, name);
triggerSetupEvent(this, {name, plugin, instance});

// We trigger the "pluginsetup" event on the player regardless, but we want
// the hash to be consistent with the hash provided for advanced plugins.
// The only potentially counter-intuitive thing here is the `instance` is the
// value returned by the `plugin` function.
this.trigger('pluginsetup', {name, plugin, instance});
return instance;
};

Expand Down Expand Up @@ -133,6 +158,8 @@ const createPluginFactory = (name, PluginSubClass) => {
PluginSubClass.prototype.name = name;

return function(...args) {
triggerSetupEvent(this, {name, plugin: PluginSubClass, instance: null}, true);

const instance = new PluginSubClass(...[this, ...args]);

// The plugin is replaced by a function that returns the current instance.
Expand All @@ -147,7 +174,10 @@ const createPluginFactory = (name, PluginSubClass) => {
*
* @mixes module:evented~EventedMixin
* @mixes module:stateful~StatefulMixin
* @fires Player#beforepluginsetup
* @fires Player#beforepluginsetup:$name
* @fires Player#pluginsetup
* @fires Player#pluginsetup:$name
* @listens Player#dispose
* @throws {Error}
* If attempting to instantiate the base {@link Plugin} class
Expand All @@ -164,12 +194,12 @@ class Plugin {
* A Video.js player instance.
*/
constructor(player) {
this.player = player;

if (this.constructor === Plugin) {
throw new Error('Plugin must be sub-classed; not directly instantiated.');
}

this.player = player;

// Make this object evented, but remove the added `trigger` method so we
// use the prototype version instead.
evented(this);
Expand All @@ -184,7 +214,7 @@ class Plugin {

// If the player is disposed, dispose the plugin.
player.on('dispose', this.dispose);
player.trigger('pluginsetup', this.getEventHash());
triggerSetupEvent(player, this.getEventHash());
}

/**
Expand Down Expand Up @@ -432,13 +462,36 @@ Player.prototype.hasPlugin = function(name) {

export default Plugin;

/**
* Signals that a plugin is about to be set up on a player.
*
* @event Player#beforepluginsetup
* @type {Plugin~PluginEventHash}
*/

/**
* Signals that a plugin is about to be set up on a player - by name. The name
* is the name of the plugin.
*
* @event Player#beforepluginsetup:$name
* @type {Plugin~PluginEventHash}
*/

/**
* Signals that a plugin has just been set up on a player.
*
* @event Player#pluginsetup
* @type {Plugin~PluginEventHash}
*/

/**
* Signals that a plugin has just been set up on a player - by name. The name
* is the name of the plugin.
*
* @event Player#pluginsetup:$name
* @type {Plugin~PluginEventHash}
*/

/**
* @typedef {Object} Plugin~PluginEventHash
*
Expand Down