Make Y.bind consistent with native bind. Fixes #2532886

[yiminghe]

http://yuilibrary.com/projects/yui3/ticket/2532886

[rgrove]

Can you describe in detail what inconsistencies these changes address? I'm curious, and it's not obvious from the code.

[juandopazo]

If I understood correctly, it's trying to fix this:

// Test in a modern browser
YUI().use('oop', function (Y) {

  function Foo(a,b,c) {
    this.a = a;
    this.b = b;
    this.c = c;
  }
  // native bind
  var Bound = Foo.bind({}, 1, 2);
  var b = new Bound(3);
  console.log(Object.keys(b));  // ['a', 'b', 'c']
  console.log(b.c); // 3

  // YUI bind
  var YBound = Y.bind(Foo, {}, 1, 2);
  var c = new YBound(3);
  console.log(Object.keys(c));  // [], but should be ['a', 'b', 'c']
  console.log(c.c); // undefined, but should be 3

});

[yiminghe]

yes，just as @juandopazo shows, maybe my user case is rare, so I totally understand if this patch can not be accepted.

[davglass]

👎 Failing tests

[juandopazo]

@davglass is Travis already building YUI before testing? I'm trying this out in my PC and after building all oop tests pass.

[juandopazo]

Is bind(0, bind, null, 0) for binding bind and/or bind.call?

[yiminghe]

bind(0, bind, null, 0 is for binding bind_, no need for bind.call.

@davglass TC passed on my pc, sorry i do not know why it failed here.

[davglass]

@juandopazo Yup!

https://travis-ci.org/yui/yui3/jobs/3264012/#L79

[jenny]

@juandopazo @rgrove Looks like tests are passing, how do you feel about this merge?

[rgrove]

I'm not opposed, although I'd kinda like to see a performance comparison. I don't think this change is worth making Y.bind() slower, so we should make sure it's still roughly as fast as the previous implementation.

[davglass]

Agreed!

[juandopazo]

It'd be nice to add my earlier example as a test, since that's what this pull request is fixing.

[davglass]

Here's a very simple perf test case that shows this new one is quite slower than the original:

http://jsperf.com/y-bind

[juandopazo]

The biggest culprit for the performance dive is binding bind_. Using Y.Array for this is anther bad idea for performance. Y.Array is mostly for forcing stuff that might be an HTMLCollection into an array. And here we know this are arguments objects that can be sliced safely.

Another thing I just realized is that Y.bind('methodName', obj) is pretty dangerous. It's not a shortcut for Y.bind(obj.methodName, obj), instead it's always looking for the latest value of the obj.methodName property. That sounds bad and slower.

I propose we go with something more like this:

var bind2 = function (method, obj) {
  // bind the method once, so the actual function
  // being bound does not change
  if (typeof method === 'string') {
    method = obj[method];
  }

  if (typeof method !== 'function') Y.log('Y.bind - what is trying to be bound is not callable', 'error');

  var args  = slice.call(arguments, 2),
      Noop  = function () {},
      bound = function () {
        return method.apply(
          // Allow the bound function to be called with "new"
          // In that case |this| will be a new object instace
          // of Noop
          this instanceof Noop ? this : obj,
          args.concat(slice.call(arguments))
        );
      };

  Noop.prototype = method.prototype;
  bound.prototype = new Noop();

  return bound;
};

[davglass]

If we are going to be modifying Y.bind I would really like to see a
shim that uses native bind when available then fall back to ours.

[rgrove]

@juandopazo I like this version a lot. +1.

@davglass Native bind may actually be slower, but it's worth testing.

[davglass]

I want to see the perf numbers from @juandopazo's bind2 and Native bind against our old one.

[ericf]

Looked at what Lodash does, saw this:
https://github.com/bestiejs/lodash/blob/master/lodash.js#L122

[juandopazo]

Wow! That sounds like a bad idea! What if a certain browser improves its
performance?

[ericf]

Yeah… Lodash is going overboard with micro-optimizations here.

[juandopazo]

@davglass Here you go http://jsperf.com/y-bind/3

[apm]

"Another thing I just realized is that Y.bind('methodName', obj) is pretty dangerous. It's not a shortcut for Y.bind(obj.methodName, obj), instead it's always looking for the latest value of the obj.methodName property. That sounds bad and slower."

This was put in as a a way to provide a way for subclasses to override early-bound methods. Please don't completely remove the old version if you choose to replace it with the slow version.

[davglass]

These new versions are way too slow for this to be merged in.

[davglass]

And a +1 to @apm's comment.

[davglass]

Closing this based on the latest Open Hours Hangout.

Since this is a breaking change and the performance is subpar, I'm closing this for now.

If anyone comes up with a better approach that is performant enough, please file a new Pull Request.