Skip to content
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

Remove nativeBind usage #2199

Merged
merged 1 commit into from
May 28, 2015
Merged

Remove nativeBind usage #2199

merged 1 commit into from
May 28, 2015

Conversation

jridgewell
Copy link
Collaborator

We cover almost all of nativeBind's cases:

  • Binding function to context
  • Binding function to context with args
  • Binding constructor with args

The only downside is supported environments will no longer get the correct length property of the bound method. Usually, you subtract the number of bound args from func's length. But, now Underscore is consistent between all environments.

http://jsperf.com/native-bind-vs-underscore/2

Re: #2197

We cover almost all of nativeBind's cases:
- Binding function to context
- Binding function to context with args
- Binding constructor with args

The only downside is supported environments will no longer get the
correct `length` property of the bound method. Usually, you subtract the
number of bound args from `func`'s length. But, now Underscore is
consistent between all environments.
@michaelficarra
Copy link
Collaborator

I'm okay with this.

edit: By the way, we can probably have additional performance gains (and avoid inconsistent behaviour between environments for sparse arrays that @jdalton always brings up) by removing the remaining native fallbacks.

@unwitting unwitting mentioned this pull request May 28, 2015
@unwitting
Copy link

Alternative in #2198 which still uses native bind and therefore would preserve arguments.length, but at massive cost compared to this approach.

See: http://jsperf.com/underscore-js-2198-vs-2199 - #2198 was twice as fast as current, but this lodash-style approach blows it out of the water.

jashkenas added a commit that referenced this pull request May 28, 2015
@jashkenas jashkenas merged commit 8ed5732 into jashkenas:master May 28, 2015
@megawac
Copy link
Collaborator

megawac commented Jun 12, 2015

This is a breaking for some constructor cases. Consider

var D = _.bind(Date, null, 2015, 05)
new D(10);

Sure this doesn't work in old environments but it may break some peoples code.

It also affects other uses such as toString

/cc @jdalton

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

Successfully merging this pull request may close these issues.

5 participants