jayphelps / core-decorators

Library of stage-0 JavaScript decorators (aka ES2016/ES7 decorators but not accurate) inspired by languages that come with built-ins like @​override, @​deprecate, @​autobind, @​mixin and more. Popular with React/Angular, but is framework agnostic.
MIT License
4.52k stars 263 forks source link

@autobind doesn't work properly when calling super from the parent's parent (and probably higher up). #69

Closed JabX closed 8 years ago

JabX commented 8 years ago
@autobind
class A {
    method() {
          console.log(this.test);
    }
}

@autobind
class B extends A {
}

@autobind
class C extends B {
    test = 'hello';

    method() {
           super.method();
     }
}

In that case, calling new C().method() logs undefined instead of 'hello'. Adding a method() definition to B (that calls super.method()) solves the problem and I'd rather not have to this. I understand this is an edge case (even more so than regular super calls that were pretty hard to solve from what I hear), but nonetheless, this isn't the expected behaviour.

jayphelps commented 8 years ago

@JabX hmmm I actually can't reproduce this one. I've added an additional test case with your code and it works as expected: #70

Can you confirm you're using one of the latest versions? v0.12.1 is the latest but I just pushed that minutes ago with an unrelated fix so v0.12.0 is fine too.

jayphelps commented 8 years ago

@JabX there were fairly recent changes to the @autobind entire class use case in #57 that I want to make sure you're testing against. (v0.12.0 contains them)

If you are indeed using the latest and it still is happening, can you let me know if you're using any babel transforms? For example, react-hot-loader does naughty things and doesn't work with autobind #48

JabX commented 8 years ago

Yup sorry about not mentioning the version, but I'm using 0.12.0

I just tried your test on my setup and it fails, returning undefined. I transpile my ES6 with the typescript compiler, maybe this is why it works for you and not for me?

jayphelps commented 8 years ago

@JabX just to clarify, you're using TypeScript and not Babel, correct? That very likely the issue. I'll play around a bit and see if I can find how TypeScript is doing things differently.

JabX commented 8 years ago

Yes that is correct. I was using Babel until recently and I'm pretty sure it transpiles classes in a very different way (to be more accurate to the ES6 spec) with loose mode off (which is the default).

jayphelps commented 8 years ago

@JabX phew! I believe I've solved the issue and published it as v0.12.2.

Please confirm.

For the curious, TypeScript does indeed transpile classes differently and in a very non-compliant way to the ES2015 (ES6) spec when it comes to super.method() calls. It simply outputs A.prototype.method.call(this) but that's not how compliant super calls work, the biggest issue being that super property lookups need to take into account getters that should be looked up and invoked with getter.call(this) which isn't done in this case, the getter is implicitly called with the wrong context.

That being said this actually revealed a bug in @autobind on the fairly edge case of when you are looking up a property directly on a prototype, but said property is not defined directly on it, but instead is up the prototype chain.

e.g.

class A {
  get method() {
    return () => {};
  }
}

class B extends A {}

A.prototype.method;
// this was handled correctly

B.prototype.method;
// but this was not, because `method` is up the chain
JabX commented 8 years ago

Wow, you already fixed it, you're definitely awesome man! I'll try as soon as I can.

EDIT : Fixed!

jayphelps commented 8 years ago

So glad. Cheers!