Fixes method-level autobind and decorate combination

[idpaterson]

I noticed some unexpected behavior when using autobind and decorate on the same method.

let callCount = 0;
class Foo() {
      @decorate(memoize)
      @autobind
      getFoo() {
        callCount++
        return this;
      }
}
const foo = Foo();

In this example, autobind works as expected, that is to say

const getFoo = foo.getFoo;
getFoo().should.equal(foo);  // and it does.

However, decorate was reapplying the decorator on every invocation. In the case of memoization, every invocation created a new function with a new cache so nothing was ever memoized properly.

const foo = new Foo();

foo.getFoo();
foo.getFoo();
foo.getFoo();

callCount.should.equal(1);  // but oops, it's 3.

I added a few tests and fixed this problem by redefining the property descriptor after the first invocation. Now getFoo() calls the memoized function which if not cached calls the bound function.

I might be missing something about why it was implemented differently before, but this approach jived with the existing tests.

Notes

I am not sure whether this problem affects anything other than decorate; unfortunately I don't have enough time or interest to dig further.

Class-level autobinding

Class-level autobinding does not work with decorate after this change nor did it work beforehand. The behavior is correct from the viewpoint of the decorator, that is to say it allows proper memoization. However, the autobinding does not work - calling the methods from a different context fails. It seems there would be some work needed there to handle the getters generated by other decorators.

To put that in terms of code, of the following test cases where FooAutobound is an @autobind class, the second test succeeds but the first fails.

describe('on methods autobound at the class level', function() {

    it('is bound to the instance', function () {
      const foo1 = new FooAutobound();
      const foo2 = new FooAutobound();

      const foo1GetFoo = foo1.getFoo;
      const foo2GetFoo = foo2.getFoo;

      const foo1Result = foo1GetFoo();
      const foo2Result = foo2GetFoo();

      foo1Result.should.not.equal(foo2Result);
      foo1Result.should.equal(foo1);
      foo2Result.should.equal(foo2);
    });

    it('is applied in a way that allows memoization', function () {
      const foo1 = new FooAutobound();
      const foo2 = new FooAutobound();

      const foo1GetFoo = foo1.getFoo;
      const foo2GetFoo = foo2.getFoo;

      const foo1Result = foo1GetFoo();
      const foo2Result = foo2GetFoo();

      foo1.getFoo();

      callCount.should.equal(2);

      foo1.getFoo(1);
      foo1.getFoo(2);

      callCount.should.equal(4);
    });
  });

Decorating getters

I took a quick look to see if this approach got us any closer to allowing getters to be decorated as requested in #65. Unfortunately it does not.

Potential solution?

Both of these seem to be particularly difficult due to the need to distinguish core-decorators getters from user space getters. core-decorators getters return a function while user space getters return any value. originalGet.call(this) gives a function if called on a core-decorators getter but you would not want to call that on a getter not created by core-decorators.

One approach that I quickly put together was to add a Symbol key to the descriptor to identify that a getter was generated by core-decorators. This allows descriptor[isCoreDecorator] ? originalGet.call(this) : originalGet, passing through the normal getter rather than calling it.

Applied within decorate this allows decoration of getters. Applied within autobind it allows getters generated by core-decorators to be distinguished and autobound. Maybe that distinction is not necessary for autobind but you would really have to go out of your way to use a getter in an unexpected context.

[idpaterson]

I think that for backwards compatibility the getter should always be writable in case anyone was using the workaround that I was using (this.foo = this.foo.bind(this); in the constructor). I updated the last commit with that change.

[jayphelps]

Thanks @idpaterson for digging into this. I will give it a full review as soon as I can. 🎉