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

Register multiple event listeners with the same callback in one call #1231

Closed
wants to merge 18 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@themicp
Contributor

themicp commented May 22, 2014

There are cases where you want the same function to be called for many different events. For example take a look at the following code from the constructor of the Loading Spinner:

    player.on('canplay', vjs.bind(this, this.hide));
    player.on('canplaythrough', vjs.bind(this, this.hide));
    player.on('playing', vjs.bind(this, this.hide));
    player.on('seeking', vjs.bind(this, this.show));
    player.on('seeked', vjs.bind(this, this.hide));
    player.on('ended', vjs.bind(this, this.hide));
    player.on('waiting', vjs.bind(this, this.show));

We want to run the function vjs.bind(this, this.hide) for 5 different events and for that purpose we call player.on 5 times which makes the code very long and ugly.

From this, I came up with this feature/enhancement where you can call vjs.on with an array of events and attach the callback in all of them with a single call (passing an event as a string still works as before). With this feature the constructor of the Loading Spinner would look like this:

    player.on(['canplay', 'canplaythrough', 'playing', 'seeked', 'ended' ], vjs.bind(this, this.hide));
    player.on(['seeking', 'error', 'waiting'], vjs.bind(this, this.show));

The same goes with vjs.one and vjs.off, too.

For the implementation of this feature I needed a method to run a function for each element of an array but Array.prototype.forEach() was not a good idea due to performance issues (http://jsperf.com/fast-array-foreach) so I created vjs.arrayForEach which does the same thing but using the for() loop.
I have also included comments, unit tests for all of my changes (including .arrayForEach()) and updated the exports file to export vjs.on, vjs.off and vjs.one to be able to access them using bracket notation from vjs. forwardMultipleEvents.

themicp added some commits May 22, 2014

Multiple event register:
Passing an array of event types in vjs.on, vjs.one or vjs.off will create a listener for each one of the given events instead of calling vjs.on multiple times with the same callback.
Create a generic function that parses the array of types given in vjs…
….on, vjs.one or vjs.off and creates a call for each type in the array
Use .forEach instead of .filter to pass through each item of the even…
…t array and write unit tests for vjs.arrayForEach to make sure that the fallback function works as expected
Export vjs.on, vjs.off, vjs.one so we can access them using bracket n…
…otation and use for() loop instead forEach() for the arrays for better performance

@mmcc mmcc added the enhancement label May 22, 2014

@mmcc

This comment has been minimized.

Show comment
Hide comment
@mmcc

mmcc May 22, 2014

Member

Thank you for submitting this! I just did a quick once over, but from what I can see this is a really clean PR with tests to boot...Awesome.

Member

mmcc commented May 22, 2014

Thank you for submitting this! I just did a quick once over, but from what I can see this is a really clean PR with tests to boot...Awesome.

@themicp

This comment has been minimized.

Show comment
Hide comment
@themicp

themicp May 22, 2014

Contributor

Thank you! I will create soon another PR implementing this enhancement in cases like the one with the Loading Spinner.

Contributor

themicp commented May 22, 2014

Thank you! I will create soon another PR implementing this enhancement in cases like the one with the Loading Spinner.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff May 22, 2014

Member

Yeah, thanks @themicp! I'm going to make a few suggested updates inline.

Hold off on the loading spinner update specifically. In #1225 we're trying to moving away from binding to multiple events there.

Member

heff commented May 22, 2014

Yeah, thanks @themicp! I'm going to make a few suggested updates inline.

Hold off on the loading spinner update specifically. In #1225 we're trying to moving away from binding to multiple events there.

Show outdated Hide outdated src/js/events.js
* @param {Function} fn Event listener.
* @param {String} method Event method (on, off, one).
*/
vjs.forwardMultipleEvents = function( elem, type, fn, method ) {

This comment has been minimized.

@heff

heff May 22, 2014

Member

By using the jsDoc comments and adding it to vjs it makes this a public function. I don't think it would get used outside of the core codebase so I think we could keep it private and allow closure compiler to mangle it. To make it public private you could change the name from vjs.forwardMultipleEvents to _forwardMultipleEvents and add the @private tag to the comment.

@heff

heff May 22, 2014

Member

By using the jsDoc comments and adding it to vjs it makes this a public function. I don't think it would get used outside of the core codebase so I think we could keep it private and allow closure compiler to mangle it. To make it public private you could change the name from vjs.forwardMultipleEvents to _forwardMultipleEvents and add the @private tag to the comment.

Show outdated Hide outdated src/js/events.js
* @param {Function} fn Event listener.
* @private
*/
vjs.on = function(elem, type, fn){
if (type instanceof Array){

This comment has been minimized.

@heff

heff May 22, 2014

Member

I'm going to pull in #1218 soon which adds a vjs.obj.isArray method, so we'll probably want to update this when that happens.

@heff

heff May 22, 2014

Member

I'm going to pull in #1218 soon which adds a vjs.obj.isArray method, so we'll probably want to update this when that happens.

Show outdated Hide outdated src/js/events.js
* @param {Function} fn Event listener.
* @private
*/
vjs.on = function(elem, type, fn){
if (type instanceof Array){
vjs.forwardMultipleEvents(elem, type, fn, 'on');
return false;

This comment has been minimized.

@heff

heff May 22, 2014

Member

Here I would just do return; instead of return false; since undefined is what gets returned otherwise.

@heff

heff May 22, 2014

Member

Here I would just do return; instead of return false; since undefined is what gets returned otherwise.

This comment has been minimized.

@heff

heff May 22, 2014

Member

actually, you could just do return vjs.forwardMultipleEvents(elem, type, fn, 'on');

@heff

heff May 22, 2014

Member

actually, you could just do return vjs.forwardMultipleEvents(elem, type, fn, 'on');

This comment has been minimized.

@themicp

themicp May 23, 2014

Contributor

Alright, noted.

@themicp

themicp May 23, 2014

Contributor

Alright, noted.

Show outdated Hide outdated src/js/exports.js
@@ -111,6 +111,10 @@ goog.exportSymbol('videojs.SubtitlesButton', vjs.SubtitlesButton);
goog.exportSymbol('videojs.CaptionsButton', vjs.CaptionsButton);
goog.exportSymbol('videojs.ChaptersButton', vjs.ChaptersButton);
goog.exportSymbol('videojs.on', vjs.on);

This comment has been minimized.

@heff

heff May 22, 2014

Member

Do you have a specific use case where you would like to use these outside of the core codebase?

So far we've avoided exposing too many top-level utility functions like this because we're not trying to make a library like jquery or lodash, but the event functions are pretty powerful so I could see exporting these as long as we can point to a reason why we're doing it.

If we do export these, we'll need to remove the @private tags from the related comments.

@heff

heff May 22, 2014

Member

Do you have a specific use case where you would like to use these outside of the core codebase?

So far we've avoided exposing too many top-level utility functions like this because we're not trying to make a library like jquery or lodash, but the event functions are pretty powerful so I could see exporting these as long as we can point to a reason why we're doing it.

If we do export these, we'll need to remove the @private tags from the related comments.

This comment has been minimized.

@gkatsev

gkatsev May 22, 2014

Member

One usecase for exporting it, is to allow videojs plugins the ability to use this so that they wont need to create their own cross-platform/browser solutions.

@gkatsev

gkatsev May 22, 2014

Member

One usecase for exporting it, is to allow videojs plugins the ability to use this so that they wont need to create their own cross-platform/browser solutions.

This comment has been minimized.

@heff

heff May 22, 2014

Member

Yeah, plugins have added a new issue to the whole utility lib argument. When you're just extending components you have access to most of these functions, e.g. Component.prototype.on(), but general plugins don't get that.

It's probably worth opening another issue to discuss these things more deeply.

  • what are the positives and negatives of exposing utility functions?
  • if we don't provide util functions, what other options do devs have?
  • are there certain classes of util functions that are more valuable than others, or harder to get externally?
    I'll open an issue.
@heff

heff May 22, 2014

Member

Yeah, plugins have added a new issue to the whole utility lib argument. When you're just extending components you have access to most of these functions, e.g. Component.prototype.on(), but general plugins don't get that.

It's probably worth opening another issue to discuss these things more deeply.

  • what are the positives and negatives of exposing utility functions?
  • if we don't provide util functions, what other options do devs have?
  • are there certain classes of util functions that are more valuable than others, or harder to get externally?
    I'll open an issue.

This comment has been minimized.

@gkatsev

gkatsev May 22, 2014

Member

Personally, I think if vjs has these utility methods, it should expose them to plugins. Why force plugin developers to rewrite stuff that's already available?
Totally agree that we should continue the discussion elsewhere.

@gkatsev

gkatsev May 22, 2014

Member

Personally, I think if vjs has these utility methods, it should expose them to plugins. Why force plugin developers to rewrite stuff that's already available?
Totally agree that we should continue the discussion elsewhere.

This comment has been minimized.

@heff
@heff

This comment has been minimized.

@themicp

themicp May 23, 2014

Contributor

I exported those functions because I couldn't access them from forwardMultipleEvents using bracket notation since the compiler was renaming them. I can probably change the code a bit to remove the exports and use a switch() on the method parameter, something like this:

switch(method){
  case 'on': 
    vjs.on( .... );
    break;
  case 'one': 
    vjs.one( .... );
    break;
  case 'off': 
    vjs.on( .... );
    break;
  default:
    break;
}
@themicp

themicp May 23, 2014

Contributor

I exported those functions because I couldn't access them from forwardMultipleEvents using bracket notation since the compiler was renaming them. I can probably change the code a bit to remove the exports and use a switch() on the method parameter, something like this:

switch(method){
  case 'on': 
    vjs.on( .... );
    break;
  case 'one': 
    vjs.one( .... );
    break;
  case 'off': 
    vjs.on( .... );
    break;
  default:
    break;
}

This comment has been minimized.

@gkatsev

gkatsev May 23, 2014

Member

Why not just give forwardMultipleEvents the function you want it to use?
So, the new method signature would be function forwardMultipleEvents(fn, event, type, callback). Then in vjs.on you'd do vjs.forwardMultipleEvents(vjs.on, event, type, fn)

@gkatsev

gkatsev May 23, 2014

Member

Why not just give forwardMultipleEvents the function you want it to use?
So, the new method signature would be function forwardMultipleEvents(fn, event, type, callback). Then in vjs.on you'd do vjs.forwardMultipleEvents(vjs.on, event, type, fn)

This comment has been minimized.

@themicp

themicp May 23, 2014

Contributor

Nice, thanks! That's how I'm going to do it.

@themicp

themicp May 23, 2014

Contributor

Nice, thanks! That's how I'm going to do it.

* Add an event listener to element
* It stores the handler function in a separate cache object
* and adds a generic handler to the element's event,
* along with a unique id (guid) to the element.
* @param {Element|Object} elem Element or object to bind listeners to
* @param {String} type Type of event to bind to.
* @param {String|Array} type Type of event to bind to.
* @param {Function} fn Event listener.
* @private

This comment has been minimized.

@heff

heff May 22, 2014

Member

If we export these we need to remove these @private tags.

@heff

heff May 22, 2014

Member

If we export these we need to remove these @private tags.

Show outdated Hide outdated src/js/lib.js
* @param {Array} array The array.
* @param {Function} fn The function to be run for each item.
*/
vjs.arrayForEach = function(array, fn) {

This comment has been minimized.

@heff

heff May 22, 2014

Member

Can we make this vjs.arr.forEach to match the vjs.obj methods? You'll need to create the vjs.arr object as well.

@heff

heff May 22, 2014

Member

Can we make this vjs.arr.forEach to match the vjs.obj methods? You'll need to create the vjs.arr object as well.

This comment has been minimized.

@themicp

themicp May 23, 2014

Contributor

Yeah, good idea I will update it soon.

@themicp

themicp May 23, 2014

Contributor

Yeah, good idea I will update it soon.

Show outdated Hide outdated src/js/lib.js
}
for (var i = 0, len = array.length; i < len; ++i) {
fn.apply( vjs, [array[i], i, array]);

This comment has been minimized.

@heff

heff May 22, 2014

Member

You could use fn.call() here an avoid making the extra array. Not sure if there's really any benefit to that though.

@heff

heff May 22, 2014

Member

You could use fn.call() here an avoid making the extra array. Not sure if there's really any benefit to that though.

This comment has been minimized.

@themicp

themicp May 23, 2014

Contributor

I thought it would be helpful for other uses of arrayForEach() to set the context to vjs. Maybe I could change that for the sake of simplicity.

@themicp

themicp May 23, 2014

Contributor

I thought it would be helpful for other uses of arrayForEach() to set the context to vjs. Maybe I could change that for the sake of simplicity.

@@ -14,6 +14,32 @@ test('should add and remove an event listener to an element', function(){
vjs.trigger(el, 'click'); // No click should happen.
});
test('should add and remove multiple event listeners to an element with a single call', function(){

This comment has been minimized.

@heff

heff May 22, 2014

Member

great tests!

@heff

heff May 22, 2014

Member

great tests!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 13, 2014

Member

@themicp Are you still interested in making some of the updates listed here? Let me know if there's any outstanding questions I can answer.

Member

heff commented Jun 13, 2014

@themicp Are you still interested in making some of the updates listed here? Let me know if there's any outstanding questions I can answer.

@themicp

This comment has been minimized.

Show comment
Hide comment
@themicp

themicp Jun 16, 2014

Contributor

@heff I am sorry for the delayed response. I am still interested in finishing this PR, I will fix those notes in the next couple of days.

Contributor

themicp commented Jun 16, 2014

@heff I am sorry for the delayed response. I am still interested in finishing this PR, I will fix those notes in the next couple of days.

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 16, 2014

Member

No problem. Sounds good, thanks.

Member

heff commented Jun 16, 2014

No problem. Sounds good, thanks.

themicp added some commits Jun 17, 2014

Created the vjs.arr object which contains all the array functions,
moved vjs.arrayForEach in the vjs.arr object and rename it to vjs.arr.forEach,
update the vjs.arr.forEach unit tests.
Code fixes in vjs.on, vjs.one and vjs.off,
renamed forwardMultipleEvents to _forwardMultipleEvents since it's a private function and also update the comments,
use vjs.arr.forEach instead of vjs.arrayForEach,
don't export vjs.one, vjs.on and vjs.off.
@themicp

This comment has been minimized.

Show comment
Hide comment
@themicp

themicp Jun 17, 2014

Contributor

I updated the PR with the things mentioned in the comments. Let me know if there is anything left.

Contributor

themicp commented Jun 17, 2014

I updated the PR with the things mentioned in the comments. Let me know if there is anything left.

@heff heff added the confirmed label Jun 23, 2014

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jun 23, 2014

Member

This looks ready to go to me. Will pull in soon.

Member

heff commented Jun 23, 2014

This looks ready to go to me. Will pull in soon.

@themicp

This comment has been minimized.

Show comment
Hide comment
@themicp

themicp Jun 23, 2014

Contributor

That's great, thank you!

Contributor

themicp commented Jun 23, 2014

That's great, thank you!

@heff

This comment has been minimized.

Show comment
Hide comment
@heff

heff Jul 3, 2014

Member

Awesome, thanks for pulling those changes in. I'll get this merged in after the holiday weekend.

Member

heff commented Jul 3, 2014

Awesome, thanks for pulling those changes in. I'll get this merged in after the holiday weekend.

@heff heff closed this in 392cbda Jul 8, 2014

paullryan added a commit to paullryan/video.js that referenced this pull request Jul 17, 2014

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