Fixes potential issues with comparator and bindAll

[brandoncarl]

In the event that a comparator function declared at instantiation is bound, these bindings can cause sort to work incorrectly. These errors can occur in IE < 9, PhantomJS, & Safari 5, or when using alternative libraries such as Lo-Dash.

In particular, this occurs if _.bindAll(this) is called in the constructor. While not good practice (and subsequently not an option in Underscore 1.5.0), it has been a prevalent anti-pattern.

This pull request increases Backbone resiliency (and compatibility with other libraries) by storing the original comparator's length during instantiation. In the event that comparator is manually updated, will default to using this.comparator.length first. Only in the event that this is falsy will this.comparatorLength be used.

[akre54]

For reference this issue is discussed more in depth at lodash/lodash#375. The simple solution, as @jdalton correctly hinted at, is to avoid _.bindAll(this) for reasons stated in jashkenas/underscore@bf657be and jashkenas/underscore@ce3d1ae.

Does this fail with latest underscore too or only Lo-Dash?

[brandoncarl]

Yes it does. @jdalton and I worked together to solve that particular issue.

At the bottom of the thread we discussed that the best solution was to patch Backbone, and I offered to write and submit the pull request.

As to whether it fails the latest underscore, that would depend on which bind was used (browser dependent), and whether the comparator was inadvertently bound. If the _.bindAll includes this.comparator, then it will break on browser versions mentioned above. The pull request basically adds a layer of resiliency and cross-library compatibility.

Hope that helps!

[akre54]

_.bindAll(this) has been discouraged for a while now and was removed from underscore a couple versions ago due to similar problems. Can you try with the _.bindAll(this, 'func1', 'comparator', 'func2') form and see if that works for you?

[brandoncarl]

Both break using this in Safari 5.0. The same should be true in PhantomJS. They work properly when the pull request is used.

On Monday, October 28, 2013 at 12:40 PM, Adam Krebs wrote:

_.bindAll(this) has been discouraged for a while now and was removed from underscore a couple versions ago due to similar problems. Can you try with the _.bindAll(this, 'func1', 'comparator', 'func2') form and see if that works for you?

—
Reply to this email directly or view it on GitHub (#2833 (comment)).

[akre54]

I'm not able to reproduce this in Safari 5. Can you attach a jsfiddle or failing test case that fails without this test but passes with?

[brandoncarl]

Honestly - I don't have time at this point. All the sample code used is in the Lo-dash issue. If you'd like to close unresolved then please feel free to do so.

The issue is definitely in place, and I think it's much better to build resiliency into the code rather than to anticipate that people won't use features like _.bindAll(this). That said, I'm not the maintainer of this repo, and I leave the decisions up to you all. Sorry I don't have more time, but between last night and today I've put 3 hours into this, and can't do any more.

[jashkenas]

This patch is a bit of false resiliency, I'm afraid.

• If you had called _.bindAll in the overridden constructor before calling super, it wouldn't have worked.
• If you had bound the comparator function externally from the class, it wouldn't have worked.

But really, this would be (not trying to be pejorative, but) a nasty little kludge to get merged. It's a hairy patch for a long discouraged practice, and moreover one that hasn't been possible for the last 4 months. Putting this out in a future version of Backbone would be a step backwards.

[brandoncarl]

Just a note on your first bullet point: the pull request affects the Backbone.Collection constructor. The change occurs during the _.extend call. As such, it doesn't matter whether super is called whatsoever.

With respect to the second point, it's not clear to me that people are going to intentionally be binding the comparator outside of the _.bindAll.

At minimum, it'd be helpful if the docs noted that the comparator function, if bound, depends on function length, which means that we don't have backwards compatibility: <IE9, Safari 5.0, etc.

[jashkenas]

Absolutely -- let's note this in the docs.

[jashkenas]

And for the record -- there's no browser compatibility issue here -- IE < 9, Safari 5, and friends all have function.length properties that work just fine.

[brandoncarl]

Btw...for the record of the record, that's not true: it's actually the native bindings that cause the problems, not the function.length properties.

Here's a codepen (fiddle not compatible with old browsers): try it in XP/Safari 5
http://codepen.io/anon/pen/qKpgn

Basically, ANY binding causes comparator problems. If you could, please do reconsider either this request, or a variation thereof.

[akre54]

The example works on Mac Safari 5 for me. CodePen is broken on XP/IE7 due to no localStorage. I'll try on IE8 later tonight.

[brandoncarl]

Sorry, I should have added that the order should be 1, 2, 4 5 when running properly.

Using Browserstack:
http://cl.ly/image/2Z0u1e0x2G0f

[akre54]

I see it in IE7 (Safari 5.1 doesn't have the problem apparently):

I think I'm going to have to agree with @jashkenas here. If you remove the bindAll call from initialize, it works as advertised, and therefore no code changes are necessary. http://codepen.io/anon/pen/xeicE.

I've written a pull to address the bindAll documentation in #2854. Let me know if it's clear enough.