Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(core) auto-bind this in class-level methods #2722

Merged
merged 1 commit into from
Sep 21, 2016
Merged

(core) auto-bind this in class-level methods #2722

merged 1 commit into from
Sep 21, 2016

Conversation

anotherchrisberry
Copy link
Contributor

Throwing this out there for discussion.

Much as I love how clean class-level methods are, they don't behave well when called as a result of a promise, e.g.

class Foo {
  this.bar = 3;
  baz() {
    console.log(this.bar)
  }
}
...
let foo = new Foo();
somePromise().then(foo.baz); // throws an exception: "cannot read property 'baz' of undefined"

There are a couple of ways to get around this:

// (1) Invoke via anonymous function
somePromise().then(() => foo.baz());

or

// (2) Declare method using arrow function, binding 'this' to the class instance
class Foo {
  this.bar = 3;
  public baz = () => {
    console.log(this.bar);
}
let foo = new Foo();
somePromise().then(foo.baz); // logs "3"

Preferences on (1) vs. (2)? I prefer (2): it's less surprising to me. But if we go with (2), I'd prefer we declare all class-level methods this way - even those that likely won't be called via a promise, or those that don't reference this in their body.

I'm not opposed to (1), and not in love with (2).

@zanthrash @icfantv @danielpeach what are your thoughts on this? Is there a better, third (or fourth) way around this?

@danielpeach
Copy link
Contributor

What about something like this:
https://github.com/andreypopp/autobind-decorator

@icfantv
Copy link
Contributor

icfantv commented Sep 21, 2016

2 feels "wrong" to me only in that it's not a natural way to write class methods. and like all the other solutions, it's a band-aid to get around an annoying limitation...but i don't have anything better to offer here yet.

1 is also annoying, but feels cleaner.

@anotherchrisberry
Copy link
Contributor Author

Looks like @danielpeach was correct, but will probably use https://github.com/jayphelps/core-decorators.js as it seems to have good support for Typescript and some other nice decorators

@icfantv
Copy link
Contributor

icfantv commented Sep 21, 2016

@anotherchrisberry just a heads up: jayphelps/core-decorators#86

@anotherchrisberry
Copy link
Contributor Author

Going to hold off on the @autobind decorator for now, since it doesn't look like it works with Typescript 2. We are using 1.8 right now but expect to move to 2 this week as we incorporate Angular 2.

We'll revisit this in a few weeks or so. In the meantime, just be aware of this little gotcha.

@anotherchrisberry anotherchrisberry merged commit 86c6d50 into spinnaker:master Sep 21, 2016
@anotherchrisberry anotherchrisberry deleted the auto-bind branch September 21, 2016 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants