_.bindAll + comparator causes breakage on function-based Backbone methods

[brandoncarl]

Got caught on this today...

If you extend a Backbone.Collection with a comparator method and use _.bindAll within the initialize function, it blows up the comparator. This does not happen when using Underscore.

While I wasn't able to completely get down to the bottom of this, a key difference occurs in Backbone.sort, where this.comparator no longer has a length of 1 (as it does in Underscore.

Wish I could easily link to the Backbone source to help identify the discrepancy. Thought I'd identify it.

[jdalton]

Can you try with the edge repo dist/ versions as well as the lodash.underscore version.

[brandoncarl]

I was using the lodash.underscore version - which edge repo dist/ version?

[jdalton]

Which ever you were using, so lodash.underscore: https://github.com/lodash/lodash/tree/master/dist

[brandoncarl]

Tried all 3: compat, standard, and underscore.

Here's the essence of the code. Sorry to post in CoffeeScript, it's what my source is written in. Can convert if it helps.

class Data extends Backbone.Collection

  # ### Constructors, destructors and comparators

  initialize : ->
    _.bindAll @
  comparator: (datum) ->
    return datum.prop

[jdalton]

Ya, converting would help. I can't read that.

[brandoncarl]

There you go. This repo is amazing btw...thanks for all of your excellent work. Hated to even pollute it with an issue!

var Data = Backbone.Collection.extend({

  initialize: function() {
    _.bindAll(this);
  },

  comparator: function(datum) {
    return datum.prop;
  }

});

[terinjokes]

I ran it through the CoffeeScript compiler to get a better idea of what it's really doing

var Data, _ref,
  __hasProp = {}.hasOwnProperty,
  __extends = function(child, parent) { for (var key in parent) { if (__hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; };

Data = (function(_super) {
  __extends(Data, _super);

  function Data() {
    _ref = Data.__super__.constructor.apply(this, arguments);
    return _ref;
  }

  Data.prototype.initialize = function() {
    return _.bindAll(this);
  };

  Data.prototype.comparator = function(datum) {
    return datum.prop;
  };

  return Data;

})(Backbone.Collection);

[jdalton]

There you go. This repo is amazing btw...thanks for all of your excellent work. Hated to even pollute it with an issue!

Thanks! Which version of Backbone are you using? I know the latest Underscore will error when you perform _.bindAll(this). Lo-Dash won't error but that shouldn't cause a problem here.

I can't see anything wrong with the generated output so I don't think the issue is with these bits.

I know Lo-Dash and later versions of Underscore perform a stable sort (which is a good thing) but may be an issue though it's a stretch.

[brandoncarl]

Good catch - it appears that Underscore has eliminated the global "_.bindAll" feature (new docs say methodNames are required).

I suspect that this change is going to require a lot of configuring as the global _.bindAll had become a standard (if poor) practice.

Nonetheless, mystery solved, and I'll proceed with more selectively binding within the collection.

Thanks!

[jdalton]

@brandoncarl Well that shouldn't cause an issue w/ Lo-Dash, as we don't throw the error. I suspect it may be changes with other api like _.after. Can you try the lodash.underscore.js version here: https://raw.github.com/lodash/lodash/1.3.1/dist/lodash.underscore.js

[brandoncarl]

That doesn't work.

The problem appears to rear its head at this point in the Backbone source:

sort: function(options) {
      if (!this.comparator) throw new Error('Cannot sort a set without a comparator');
      options || (options = {});

      if (_.isString(this.comparator) || this.comparator.length === 1) {
        this.models = this.sortBy(this.comparator, this);
      } else {
        this.models.sort(_.bind(this.comparator, this));
      }

      if (!options.silent) this.trigger('sort', this, options);
      return this;
    }

When using Underscore 1.4.3, this.comparator refers to the correct comparator function (as coded above). When using Lo-dash, it appears to report a form of bound:

// Beginning of this.comparator using Lo-dash, referred to as "bound" in DevTools
// References: lodash.underscore.js:681
function () {
        // `Function#bind` spec
        // http://es5.github.io/#x15.3.4.5

Are the bound functions stored as a wrapped form of the function perhaps, not triggering this.comparator.length == 1 (which does trigger in Underscore).

[jdalton]

When using Underscore 1.4.3, this.comparator refers to the correct comparator function (as coded above).

I know Underscore 1.4.4 broke their _.bind method but their 1.4.3 should be fine. Are you sure you weren't using Underscore 1.4.4?

[brandoncarl]

Just checked...definitely 1.4.3

[brandoncarl]

I am fine with the solution being "don't use a global _.bindAll when functions may be impacted". Also happy to keep helping if you want to fully resolve.

[jdalton]

I'd like to figure this out ;)

The _.bindAll(this) would bind this.comparator too so I can see why Lo-Dash is returning a bound method. Try loading the page in Firefox latest and see if it still has the same problem. If it does then try this, before you include lodash or backbone add this script, <script>Function.prototype.bind = null;</script>, and see if underscore 1.4.3 still works for you.

[brandoncarl]

Works on Firefox latest. When I add the <script> Firefox no longer works properly, nor does Chrome.

[jdalton]

Ahh ok now! This is a problem w/ native bind vs the fallback. So after the <script>Function.prototype.bind = null;</script> script add es5-shim <script src="http://cdnjs.cloudflare.com/ajax/libs/es5-shim/2.1.0/es5-shim.js"></script> and try again in Chrome/Firefox.

[brandoncarl]

Adding that leaves it broken in both.

[brandoncarl]

Here's a fix for this. If Backbone.sort is changed from:

if (_.isString(this.comparator) || this.comparator.length === 1)

to

if (_.isString(this.comparator) || this.comparator.length <= 1) {

things work fine.

Basically, the Lo-dash bound function has implicit arguments (and thus function.length is 0), while the Underscore bound function retains explicit arguments (and thus function.length is, in my case, 1).

Did that all make sense?

[brandoncarl]

This appears to be the case because Backbone's sort is overloaded: http://backbonejs.org/#Collection-sort

The sort function can be a string, take one, or two arguments. Since the Lo-dash binding doesn't set the "length" of the function (number of arguments), we run into errors.

[jdalton]

This is an issue with the assumed behavior of a bound function. Fallbacks and non-native alternatives do not set the length of the bound function. The reason you see the error in Lo-Dash in Chrome is because Lo-Dash uses a fallback when in V8 because it is considerably faster. You'll also have issues in IE < 9, PhantomJS, and a few others that like native Function#bind.

[brandoncarl]

Gotcha - so I think that leaves us with two things:

1. In Backbone, the number of comparator arguments should be stored separate from the bound function. Then, function.length would not need to be called.
2. Is it possible to create non-native fallbacks that set number of arguments/function.length?

[jdalton]

2 Is it possible to create non-native fallbacks that set number of arguments/function.length?

It is but generally no one attempts it because it's a bit hacky. You won't find it done in Underscore/ES5-Shim/MDN. It would require using eval or Function(...) to compile a function which means it wouldn't work in CSP restricted environments though those environments have native bind so it may not be an issue.

[brandoncarl]

Alright then...this is an easy fix for Backbone. I'll submit a pull request.

[jdalton]

@brandoncarl Cool! When submitting it to Backbone appeal to IE < 9, PhantomJS, & Safari 5 support as they may not budge for a Lo-Dash issue. They may just suggest not using _.bindAll(this) :3

[jdalton]

@brandoncarl Ok so starting in v2.3.0 we will no longer be using native Function#bind. This will ensure consistent behavior in old and new environments and align better with our _.partial and _.curry implementations which don't set the length of the created function.

[brandoncarl]

That's a great idea, and I like the added consistency as well.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.