Skip to content
This repository has been archived by the owner on Sep 29, 2020. It is now read-only.

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

Closed
JabX opened this issue Apr 13, 2016 · 8 comments

Comments

@JabX
Copy link

JabX commented Apr 13, 2016

@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 added a commit that referenced this issue Apr 13, 2016
@jayphelps
Copy link
Owner

@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
Copy link
Owner

@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
Copy link
Author

JabX commented Apr 13, 2016

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
Copy link
Owner

@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
Copy link
Author

JabX commented Apr 13, 2016

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 added a commit that referenced this issue Apr 13, 2016
jayphelps added a commit that referenced this issue Apr 13, 2016
catch another prototype.key case for autobind, fixes #69
@jayphelps
Copy link
Owner

@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
Copy link
Author

JabX commented Apr 14, 2016

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

EDIT : Fixed!

@jayphelps
Copy link
Owner

So glad. Cheers!

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

No branches or pull requests

2 participants