Shopify / tslint-config-shopify

Shopify’s TypeScript rules and configs.
https://www.npmjs.com/package/tslint-config-shopify
37 stars 3 forks source link

Flag unbound method references #41

Closed GoodForOneFare closed 7 years ago

GoodForOneFare commented 7 years ago

Like no-unbound-method on steroids:

This is fine:

class Foo { 
  constructor() {
    this.bar = this.bar.bind(this);
  }

  bar() {}

  qux() {
    requestAnimationFrame(this.bar); // this.bar was bound in the constructor; all good!
  }
}

This is fine:

class Foo { 
  constructor() {
  }

  @autobind // 
  bar() {}

  qux() {
    requestAnimationFrame(this.bar); // this.bar was autobound; all good!
  }
}

This is bad:

class Foo { 
  constructor() {
  }

  bar() {}

  qux() {
    requestAnimationFrame(this.bar); // no constrcutor binding; no autobind; bad!
  }
}
GoodForOneFare commented 7 years ago

@lemonmade would this rule be useful? Can we just enforce @autobind everywhere?

lemonmade commented 7 years ago

I think it's not universally applicable. It is never needed for React lifecycle hooks (most notably, render), and not needed if a function actually doesn't use this (unlikely and undesirable, but occasionally needed), and not needed for methods that are never passed as callbacks. IMO, too many potential issues to make it a rule.