Add option to bind multiple events #640

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet

Updated the Backbone.Events.bind method to accept multiple events to bind to, which allows the following syntax:

collection.bind('add remove', this.onCollectionSizeChanged);

The multiple events are space delimited.

+1 I'd definitely use this.

+1 from me as well.

Don't forget to apply the same pattern to the unbind method. Inside a view you might want to unbind on destroy:


Maybe with an alternate syntax of comma separated events :
collection.bind('add,remove', this.onCollectionSizeChanged);

@pixelastic I like you suggestion to use comma separated event names

Collaborator

wookiehangover commented Oct 30, 2011

hmm, I think comma separating multiple event bindings bucks established conventions from jQuery and other libs. Given that there's traction towards offering even more API parity with jQuery events (#641), I think it'd be best to follow their lead and stick with space delimited events.

Collaborator

wookiehangover commented Oct 30, 2011

@jashkenas care to chime in on this one?

I'm good with space delimited events. I'll make the change and add it to the pull request

You may still want to keep the regular expression. Imagine a b c (2 spaces between a and b). Typos happen.

evs = evs.split(/ +/g);

+1 from me as well.

Contributor

lennym commented Nov 16, 2011

+1 from me too.

eric-hu commented Nov 18, 2011

+1 from me--used bind thinking Backbone's bind is jQuery's.

+1

I seem to have to work around this issue a lot.

I will say, if we're going to allow passing multiple events to a the bind() method, then the trigger() method should supply the name of the event that was triggered. Sometimes my callback needs to vary slightly depending on the event.

For example, I might apply effect transitions to an element if the callback is trigger via a normal add/remove type event, but not during initialization (or rendering) of the object.

The problem is that will require changing the trigger() method. I think the cleanest solution would be to make the trigger() event always pass an event object as the first argument--but that obviously changes the API and breaks existing code.

Right now, my solution has been to do something like:

var binder = function (type, callback){
    var type = type;
    return function (){
        var args = Array.prototype.slice.call(arguments);
        args.unshift(type);
        return callback.apply(this, args);
    };
}

var callback = function (type){
    console.log(type);
}

view.bind("removed", binder("removed", callback));
view.bind("render", binder("render", callback));

This allows me to write a single callback function that I can bind to multiple events. It's just cumbersome.

Contributor

lennym commented Nov 18, 2011

@dswitzer Can you not achieve exactly that by binding onto 'all' and then filtering the event type accordingly?

@lennym:

You could, but I have complex model/views that fire lots of events. So triggering the function to run on each trigger is inefficient.

I don't have access to Git at the moment, but here's the code I'm really using to give me a bindMany:

var Backbone = (function (Backbone){
    var EventsAddons = {
        // allows multiple events to be bound to a single callback and always passes an event type as the first argument
        bindMany: function (ev, callback, context){
            ev = !_.isArray(ev) ? ev.split(/ +/g) : ev;
            for( var i=0, evl = ev.length; i < evl; i++ ){
                this.bind(ev[i], binder(ev[i], callback), context);
            }
        }       

        // allows unbinding multiple events
        , unbindMany: function (ev, callback){
            ev = !_.isArray(ev) ? ev.split(/ +/g) : ev;
            for( var i=0, evl = ev.length; i < evl; i++ ){
                var context = (!callback) ? null : binder(ev[i], callback);
                this.unbind(ev[i], context);
            }
        }       
    };

    // callback closure that always passes the event type to the callback
    function binder(type, callback){
        return function (){
            var args = Array.prototype.slice.call(arguments);
            args.unshift(type);
            return callback.apply(this, args);
        };
    };

    // extend the events
    _.extend(Backbone.Events, EventsAddons);
    // add the new methods to the other Backbone models
    _.extend(Backbone.Model.prototype, EventsAddons);
    _.extend(Backbone.Collection.prototype, EventsAddons);
    _.extend(Backbone.Router.prototype, EventsAddons);
    _.extend(Backbone.View.prototype, EventsAddons);

    return Backbone;
})(Backbone || {});

@dswitzer If your callback functions differently based upon the event that was triggered, then I would argue you should have a different callback for each trigger.

I really don't want to change the API for the trigger event and break the backwards compatibility.

@pauloder:

While I certainly understand that point of view, there are always exceptions. A common use case I run into where I want to attach the same basic callback to multiple events is when rendering a view.

I normally have a fire off a render and change event with my views. During the "render" event, I do not want any effects to work on a timed delay--I want everything to be instantaneous. However, during the "change" event, I might want some effects applied to help draw attention to the screen.

So, the only logic that's different is a variable I use to determine the effects delay. This is a case where knowing the event name is really handy. Are there different ways to do this, sure, but it adds more code and complexity (IMO.)

Maybe the best option in the meantime (especially if you decide to change the name of bind/unbind to on/off) would be to add new methods for binding multiple events that always pass in the event name as the first argument.

@dswitzer that makes sense.

I'm open to tweaking this patch.

I would really like to hear from one of the project maintainers to get their feedback too. So far I haven't heard anything, and I would like to hear from them before I spend more time on this patch

Owner

jashkenas commented Nov 21, 2011

Sorry -- been busy. I'll try to take a look when we go through the next big ticket sweep.

+1 on this.

Contributor

davidgtonge commented Dec 5, 2011

+1 - would definitely use

+1 for sure

I'd like to see this pulled into the core.

jashkenas closed this in 5aa4fda Jan 13, 2012

Owner

jashkenas commented Jan 13, 2012

Support for this is now on master with the above commit. Take the time to kick the tires if you have a minute.

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