Skip to content

Commit

Permalink
Simplifying _.bind to remove ES5 edge cases
Browse files Browse the repository at this point in the history
  • Loading branch information
jashkenas committed Jan 30, 2013
1 parent 89060f0 commit ce3d1ae
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 14 deletions.
3 changes: 0 additions & 3 deletions test/functions.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,7 @@ $(document).ready(function() {
// To test this with a modern browser, set underscore's nativeBind to undefined
var F = function () { return this; };
var Boundf = _.bind(F, {hello: "moe curly"});
var newBoundf = new Boundf();
equal(newBoundf.hello, undefined, "function should not be bound to the context, to comply with ECMAScript 5");
equal(Boundf().hello, "moe curly", "When called without the new operator, it's OK to be bound to the context");
ok(newBoundf instanceof F, "a bound instance is an instance of the original function");
});

test("bindAll", function() {
Expand Down
14 changes: 3 additions & 11 deletions underscore.js
Original file line number Diff line number Diff line change
Expand Up @@ -582,18 +582,10 @@
// Delegates to **ECMAScript 5**'s native `Function.bind` if available.
// We check for `func.bind` first, to fail fast when `func` is undefined.
_.bind = function(func, context) {
var args, bound;
if (func.bind === nativeBind && nativeBind) return nativeBind.apply(func, slice.call(arguments, 1));
if (!_.isFunction(func)) throw new TypeError;
args = slice.call(arguments, 2);
return bound = function() {
if (!(this instanceof bound)) return func.apply(context, args.concat(slice.call(arguments)));
ctor.prototype = func.prototype;
var self = new ctor;
ctor.prototype = null;
var result = func.apply(self, args.concat(slice.call(arguments)));
if (Object(result) === result) return result;
return self;
var args = slice.call(arguments, 2);

This comment has been minimized.

Copy link
@jdalton

jdalton Jan 30, 2013

Contributor

I'm pretty sure this commit introduces a compat issue with a-popular-Backbone-pattern:
(this applies to version <=0.9.9, as 0.9.10 introduces a similar compat issue)

// please excuse any nits in the example code below,
// I'm using one from my own issue tracker
var model =  Backbone.Model.extend({});
var collection = Backbone.Collection.extend({
    model: model,
    initialize: function() {
        _.bindAll(this);
    }
})

collection.add({ name : 'test' });
// Uncaught TypeError: Object [object Object] has no method 'set' 

Now older IE and WebKit browsers without native Function#bind will error.
I ran into this compat issue as well, back in the early days of my own lib, that caused people to temporarily revert.

This comment has been minimized.

Copy link
@caseywebdev

caseywebdev Jan 30, 2013

Contributor

I think really think _.bindAll needs to go away...

This comment has been minimized.

Copy link
@gsamokovarov

gsamokovarov Jan 30, 2013

Contributor

👍 It caused nothing, but trouble for me.

return function() {
return func.apply(context, args.concat(slice.call(arguments)));
};
};

Expand Down

10 comments on commit ce3d1ae

@braddunbar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this commit in response to? Is there a bug report I can look at that describes the issue?

@jdalton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The on a whim discarding of cross-browser support is concerning and in direct contradiction to @jashkenas earlier comments (1-24-2013):

Underscore should always be a simple, single script that supports all of the environments you're likely to encounter as a JavaScript developer out of the box. Code that is written to work against one version of Underscore should just work, cross-platform.

I believe Underscore is the only mainstream lib that doesn't cover this case in the Function#bind fallback :'(

@jashkenas
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@braddunbar -- The commit's not in response to any issue. It's just in response to noticing, while adding partial, that all of the constructor-related junk was still present in the bind fallback implementation.

It would be great if we could aim towards the straightforward, common-sense implementations of Underscore functions, and take pains to avoid edge-case code paths when those particular edge cases aren't useful ones that you'd ever want to intentionally take. In general, binding a constructor function is an error -- we might want to make it harder to accidentally do, instead of going out of our way to allow it.

@jdalton
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jashkenas

In general, binding a constructor function is an error -- we might want to make it harder to accidentally do, instead of going out of our way to allow it.

It's great that you're opinionated about the language, but I think that's better suited for CoffeeScript.

Dropping support for this in the fallback, while it's still allowed in modern browsers isn't the way to make it harder to accidentally do. Instead, devs are unaware of your policy until they begin back-compat testing their, until then, working code. At that point the inconsistencies may not be easy to diagnose (older browsers don't have the best debug tools). It would be great if code written against one version of Underscore would work cross-platform.

@braddunbar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for elaborating @jashkenas! I agree that constructor related edge cases usually indicate a misunderstanding if not a bug. In fact, there are already other discrepancies with the fallback that cannot be fixed. For instance, the issue described in jashkenas/backbone#2195 arises because _.bind (or __bind) does not correctly set the length property like Function#bind.

That said, I don't think the edge cases or their validity are at issue here. The problem is that they won't arise until someone stumbles across a browser without support for Function#bind. Making it harder to accidentally bind a constructor function is fine, but I don't think this patch accomplishes that since most developers only test in older browsers as an afterthought, if at all.

I understand the implications and incorrectness of binding a constructor, but I've stumbled across this type of problem in the wild before support was added and I'd rather not debug it again because support was dropped, good practice or not. Further, I don't think it's reasonable to expect that all users of the library know and understand these differences and my guess is that the vast majority of them do not.

@domenic
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am +1 on removing the nativeBind usage altogether, as the desire to simplify makes sense, but the cross-browser inconsistencies are troublesome. We bind constructors somewhat frequently as a means of creating factory functions with pre-filled parameters.

In general I think removing the nativeXyz usages in these cases would be nice. The only downside I can see is that it creates more noise in the debugger, since underscore code doesn't get the invisible-to-step-through privileges of native implementations.

@phated
Copy link

@phated phated commented on ce3d1ae Jan 31, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the point in shooting down an issue that asked underscore to remove old IE support and then remove old IE support in a single function and call it an "edge case". Reduce library size and increase consistency by just dropping those old browsers.

@jashkenas
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand the implications and incorrectness of binding a constructor, but
I've stumbled across this type of problem in the wild before support was added

Yep -- I have as well. Let's try to tackle it from the other end by making it harder to accidentally bind a constructor. To that end, I've removed the _.bindAll(object) form. Folks who have a naked object and really wish to bind every value can use bindAll in conjunction with _.functions.

We bind constructors somewhat frequently as a means of creating factory
functions with pre-filled parameters.

... but you don't really "bind" the constructors in the sense of setting their this -- you want to fill in a couple of arguments. Better to use partial for that, no?

@jdalton
Copy link
Contributor

@jdalton jdalton commented on ce3d1ae Feb 1, 2013

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jashkenas

We bind constructors somewhat frequently as a means of creating factory
functions with pre-filled parameters.

... but you don't really "bind" the constructors in the sense of setting their this -- you want to fill in a couple of arguments. Better to use partial for that, no?

In the case of using ES5 bind on a constructor, it would be kind of like partial in that it's not enforcing its given thisArg binding. Even then, Underscore's _.partial won't work for @domenic's use case. I could easily see someone wanting to create different flavors of a constructor by passing predefined options. In this case though, Underscore 1.4.4's _.bind (in older browsers) and _.partial (in any) won't work.

I'm not a fan of Underscore becoming Crockford-like, trying to enforce (inconsistently or not) whatever it sees as the "good parts". I think a utility lib should add utility, not introduce last minute breaking back-compat changes w/o consulting its core devs or users :(

@braddunbar
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep -- I have as well. Let's try to tackle it from the other end by making it harder to accidentally bind a constructor. To that end, I've removed the _.bindAll(object) form. Folks who have a naked object and really wish to bind every value can use bindAll in conjunction with _.functions.

That's fine, and I agree that _.bindAll(obj) is a bad pattern, but it's orthogonal to the cross browser consistency of _.bind. From my perspective, there are two possibilities.

  1. Function#bind is a useful utility that is used by _.bind when available and polyfilled to the degree possible when not.
  2. The behavior of Function#bind is considered harmful or incorrect and _.bind chooses to take a separate route in all browsers, regardless of support.

Instead, this patch implies that the behavior of Function#bind is useful in browsers that support it but considered harmful or incorrect in browsers that don't. If our stance is that _.bind should never be used for constructors then let's remove the native guard as well.

Please sign in to comment.