Permalink
Browse files

Removing the ability for bindAll to be called with no arguments. Whit…

…e-listing the function names to be bound is always a better option.
  • Loading branch information...
jashkenas committed Feb 1, 2013
1 parent 0fb9c99 commit bf657be243a075b5e72acc8a83e6f12a564d8f55
Showing with 6 additions and 3 deletions.
  1. +5 −2 test/functions.js
  2. +1 −1 underscore.js
View
@@ -63,9 +63,12 @@ $(document).ready(function() {
getName : function() { return 'name: ' + this.name; },
sayHi : function() { return 'hi: ' + this.name; }
};
_.bindAll(moe);
raises(function() { _.bindAll(moe); }, Error, 'throws an error for bindAll with no functions named');
_.bindAll(moe, 'sayHi');
curly.sayHi = moe.sayHi;
equal(curly.sayHi(), 'hi: moe', 'calling bindAll with no arguments binds all functions to the object');
equal(curly.sayHi(), 'hi: moe');
});
test("memoize", function() {
View
@@ -598,7 +598,7 @@
// all callbacks defined on an object belong to it.
_.bindAll = function(obj) {
var funcs = slice.call(arguments, 1);
if (funcs.length === 0) funcs = _.functions(obj);
if (funcs.length === 0) throw new Error("bindAll must be passed function names");
each(funcs, function(f) { obj[f] = _.bind(obj[f], obj); });
return obj;
};

8 comments on commit bf657be

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Feb 7, 2013

Is this actually necessary? It's a huge BC break.
I don't see the benefits after all. Why did you change the default behavior?

Now it's _.bindAll(this, _.functions(this));, is there something broken with that code?

havvg replied Feb 7, 2013

Is this actually necessary? It's a huge BC break.
I don't see the benefits after all. Why did you change the default behavior?

Now it's _.bindAll(this, _.functions(this));, is there something broken with that code?

@braddunbar

This comment has been minimized.

Show comment
Hide comment
@braddunbar

braddunbar Feb 7, 2013

Collaborator

Hi @havvg! There is already some discussion on this topic in ce3d1ae. Check it out and let us know what you think.

Collaborator

braddunbar replied Feb 7, 2013

Hi @havvg! There is already some discussion on this topic in ce3d1ae. Check it out and let us know what you think.

@jdalton

This comment has been minimized.

Show comment
Hide comment
@jdalton

jdalton Feb 7, 2013

Contributor

@havvg

Now it's _.bindAll(this, _.functions(this));, is there something broken with that code?

It's worse. _.bindAll(this, _.functions(this)); will also error "Cannot read property 'bind' of undefined". To get the same functionality you're looking for you'll have to do _.bindAll.apply(_, [this].concat(_.functions(this))) :(

Contributor

jdalton replied Feb 7, 2013

@havvg

Now it's _.bindAll(this, _.functions(this));, is there something broken with that code?

It's worse. _.bindAll(this, _.functions(this)); will also error "Cannot read property 'bind' of undefined". To get the same functionality you're looking for you'll have to do _.bindAll.apply(_, [this].concat(_.functions(this))) :(

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Feb 7, 2013

Yeah, I noticed the "simple" version did not work out, so I reverted my upgrade of backbone and underscore for now.
I read the discussion on the other commit, but I'm not that into Javascript, to have an opinion on that native bind thing.

Thank you very much for the code sample!
I will wait until the discussion has a final statement and an upgrade manual on how to replace the _.bindAll(this); on my Backbone.Views and "plain"/"naked" objects.

havvg replied Feb 7, 2013

Yeah, I noticed the "simple" version did not work out, so I reverted my upgrade of backbone and underscore for now.
I read the discussion on the other commit, but I'm not that into Javascript, to have an opinion on that native bind thing.

Thank you very much for the code sample!
I will wait until the discussion has a final statement and an upgrade manual on how to replace the _.bindAll(this); on my Backbone.Views and "plain"/"naked" objects.

@caseywebdev

This comment has been minimized.

Show comment
Hide comment
@caseywebdev

caseywebdev Feb 7, 2013

Contributor

@havvg why not just use listenTo in your views? The callbacks are given the view's context that way and you don't have to worry about the ugly _.bindAll(this).

Contributor

caseywebdev replied Feb 7, 2013

@havvg why not just use listenTo in your views? The callbacks are given the view's context that way and you don't have to worry about the ugly _.bindAll(this).

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Feb 7, 2013

Hm.. didn't think about it, yet. I will definitely check this one.

What about "foreign" listeners, e.g. Google Maps API: google.maps.event.addListener(this.marker, 'dragend', this.onMarkerDragend);
this being a Backbone.View again. I still need a bind for that, don't I? That's why bindAll was just practical for all cases so far, but maybe I missed something :)

havvg replied Feb 7, 2013

Hm.. didn't think about it, yet. I will definitely check this one.

What about "foreign" listeners, e.g. Google Maps API: google.maps.event.addListener(this.marker, 'dragend', this.onMarkerDragend);
this being a Backbone.View again. I still need a bind for that, don't I? That's why bindAll was just practical for all cases so far, but maybe I missed something :)

@caseywebdev

This comment has been minimized.

Show comment
Hide comment
@caseywebdev

caseywebdev Feb 7, 2013

Contributor

Yes you'd need to bind that one.

google.maps.event.addListener(this.marker, 'dragend', _.bind(this.onMarkerDragend, this));
// or
var self = this;
google.maps.event.addListener(this.marker, 'dragend', function (ev) { self.onMarkerDragend(ev); });
// this is a bit faster, but less terse

But at least you're avoiding the "double-binding" that's going on behind then scene when you can use listenTo with the view's context.

this.listenTo(this.model, 'throwParty', this.dance);
// behind the scenes you get view.dance.apply(view, args...)

where as with bindAll it looks more like...

_.bindAll(this, 'dance');
// now view.dance is really function () { view.dance.apply(view, args...); }
// so when you use it as a callback...
this.listenTo(this.model, 'throwParty', this.dance);
// you end up with essentially (function () { view.dance.apply(view, args...); }).apply(view, args...);

A bit redundant, and since you can no longer access the original function that was bindAlled, you can't get around this.

Contributor

caseywebdev replied Feb 7, 2013

Yes you'd need to bind that one.

google.maps.event.addListener(this.marker, 'dragend', _.bind(this.onMarkerDragend, this));
// or
var self = this;
google.maps.event.addListener(this.marker, 'dragend', function (ev) { self.onMarkerDragend(ev); });
// this is a bit faster, but less terse

But at least you're avoiding the "double-binding" that's going on behind then scene when you can use listenTo with the view's context.

this.listenTo(this.model, 'throwParty', this.dance);
// behind the scenes you get view.dance.apply(view, args...)

where as with bindAll it looks more like...

_.bindAll(this, 'dance');
// now view.dance is really function () { view.dance.apply(view, args...); }
// so when you use it as a callback...
this.listenTo(this.model, 'throwParty', this.dance);
// you end up with essentially (function () { view.dance.apply(view, args...); }).apply(view, args...);

A bit redundant, and since you can no longer access the original function that was bindAlled, you can't get around this.

@havvg

This comment has been minimized.

Show comment
Hide comment
@havvg

havvg Feb 8, 2013

Thank you for that insight!

havvg replied Feb 8, 2013

Thank you for that insight!

Please sign in to comment.