New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use Function.prototype.bind if available #1632

Closed
daffl opened this Issue Apr 20, 2015 · 5 comments

Comments

Projects
None yet
4 participants
@daffl
Contributor

daffl commented Apr 20, 2015

Currently it always returns a wrapper function (see https://github.com/bitovi/canjs/blob/master/util/jquery/jquery.js#L98)

@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten Apr 21, 2015

@daffl
The risk in using Function.prototype.bind 'if available', is that it may have been polyfilled. (E.g. Modernizr contains a polyfill for it, built into the library.)

The reason CanJS has its own proxy method instead of straight-up re-using jQuery is the fact that the argument checking and currying that is part of jQuery's proxy method was deemed too detrimental to performance, because CanJs heavily leans on proxy internally and calls it many times.

A fully compliant polyfill for Function.prototype.bind would be just as stringent in its checks (if not even moreso) than jQuery's proxy method and would undoubtedly lead to comparable performance characteristics and thus the same performance problems...

Currently it always returns a wrapper function

Nitpick: technically, Function.prototype.bind also returns a wrapping function...

function foo() {};
console.log(foo == foo.bind(this)); // false

rjgotten commented Apr 21, 2015

@daffl
The risk in using Function.prototype.bind 'if available', is that it may have been polyfilled. (E.g. Modernizr contains a polyfill for it, built into the library.)

The reason CanJS has its own proxy method instead of straight-up re-using jQuery is the fact that the argument checking and currying that is part of jQuery's proxy method was deemed too detrimental to performance, because CanJs heavily leans on proxy internally and calls it many times.

A fully compliant polyfill for Function.prototype.bind would be just as stringent in its checks (if not even moreso) than jQuery's proxy method and would undoubtedly lead to comparable performance characteristics and thus the same performance problems...

Currently it always returns a wrapper function

Nitpick: technically, Function.prototype.bind also returns a wrapping function...

function foo() {};
console.log(foo == foo.bind(this)); // false
@daffl

This comment has been minimized.

Show comment
Hide comment
@daffl

daffl Apr 21, 2015

Contributor

Good point. I didn't remember the reason for our own proxy. We should probably leave it then although it would be interesting to see if there is a performance difference between Function.prototype.bind and your own wrapper.

Contributor

daffl commented Apr 21, 2015

Good point. I didn't remember the reason for our own proxy. We should probably leave it then although it would be interesting to see if there is a performance difference between Function.prototype.bind and your own wrapper.

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 13, 2015

Contributor

I think we should make this change. If some project is accidentally picking up Modernizr's .bind, that project can pretty easily patch can.proxy.

Yes, jQuery's .proxy was slow, but not the bottleneck in performance.

The BIG reason for this is debugging. If you look at almost any function in CanJS, you get back a can.proxy generated function. Instead if Function.prototype.bind is used, you will see your original source function.

IMO, the trade offs are worth it.

Contributor

justinbmeyer commented May 13, 2015

I think we should make this change. If some project is accidentally picking up Modernizr's .bind, that project can pretty easily patch can.proxy.

Yes, jQuery's .proxy was slow, but not the bottleneck in performance.

The BIG reason for this is debugging. If you look at almost any function in CanJS, you get back a can.proxy generated function. Instead if Function.prototype.bind is used, you will see your original source function.

IMO, the trade offs are worth it.

@justinbmeyer justinbmeyer added this to the 2.2.6 milestone May 13, 2015

@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten May 14, 2015

Instead if Function.prototype.bind is used, you will see your original source function.

Fyi: maybe that's the case with Chrome and its development tools, but in Firefox with Firebug you get back a different function with its source display set to [native code] and no link to the location where the original function is defined.

If debugging is the big reason that makes it worth the trade-off, perhaps you want to consider that it won't improve the experience for everyone. Only for those debugging via Chrome. (And the behavior may change there at some point as well. It's obviously not part of a standard.)

rjgotten commented May 14, 2015

Instead if Function.prototype.bind is used, you will see your original source function.

Fyi: maybe that's the case with Chrome and its development tools, but in Firefox with Firebug you get back a different function with its source display set to [native code] and no link to the location where the original function is defined.

If debugging is the big reason that makes it worth the trade-off, perhaps you want to consider that it won't improve the experience for everyone. Only for those debugging via Chrome. (And the behavior may change there at some point as well. It's obviously not part of a standard.)

@daffl daffl modified the milestones: 2.3.0, 2.2.6 May 14, 2015

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer May 14, 2015

Contributor

@rjgotten I was aware of the issue with Firebug, but I still think it's worth it. In my trainings, I think over 90% of people use chrome now.

Perhaps for other devs, we can put an __proxied property on the returned function that points to the original function.

Contributor

justinbmeyer commented May 14, 2015

@rjgotten I was aware of the issue with Firebug, but I still think it's worth it. In my trainings, I think over 90% of people use chrome now.

Perhaps for other devs, we can put an __proxied property on the returned function that points to the original function.

daffl added a commit that referenced this issue May 15, 2015

@daffl daffl closed this in #1701 May 15, 2015

justinbmeyer added a commit that referenced this issue May 18, 2015

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