jashkenas / underscore

JavaScript's utility _ belt
https://underscorejs.org
MIT License
27.33k stars 5.53k forks source link

_.bind nontrivial constructor fix (?) #2845

Open jgonggrijp opened 4 years ago

jgonggrijp commented 4 years ago

A quick summary of what led up to this diff:

I'm not actually sure this should be merged. Is using a bound function as a constructor important to support? On the other hand, #2199 was probably the wrong solution to the wrong problem in the first place, so maybe this detour should never have been made.

jashkenas commented 4 years ago

I'm not actually sure this should be merged. Is using a bound function as a constructor important to support?

It is not important to support. Within a constructor function, this should be the instance you’re constructing.

I feel like we should rely on native bind here if there are performance improvements it gives us, and not worry about it otherwise.

jgonggrijp commented 4 years ago

Here's the thing though. executeBound already has a fallthrough case for being called as a constructor (i.e. with new), in which case it doesn't keep the this binding but does keep the argument bindings:

https://github.com/jashkenas/underscore/blob/4334c126b3e71e4aeb8912d05d72139d24f7d0f1/modules/index.js#L770-L778

And it works for simple constructors, but not for fancy stuff like Date. Native bind has this sophistication too, but it does work for fancy constructors as well. So where to take this:

jashkenas commented 4 years ago

So where to take this:

  • Support some constructors but not others (current situation)?
  • Support all constructors (this PR)?
  • Support no constructors at all (another breaking change)?

I don’t believe that it’s terribly important — these sorts of tickets are originally driven by test-case, nit-picked development, not real-world use cases ... But, if it doesn’t cost us much (in bound function call speed, and in file size) to support all constructors with this PR, I think it’s fine and good to merge it!

jgonggrijp commented 4 years ago

Time for another benchmark, then.