Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Rule suggestion: static-this #4428

Closed
JoshuaKGoldberg opened this issue Jan 2, 2019 · 7 comments
Closed

Rule suggestion: static-this #4428

JoshuaKGoldberg opened this issue Jan 2, 2019 · 7 comments

Comments

@JoshuaKGoldberg
Copy link
Contributor

JoshuaKGoldberg commented Jan 2, 2019

Rule Suggestion

Is your rule for a general problem or is it specific to your development style?

General problem

What does your suggested rule do?

Ban the use of this in static methods.

Static this usage can be confusing for newcomers.
It can also become imprecise when used with extended classes when a static this of a parent class no longer specifically refers to the parent class.

List several examples where your rule could be used

Instead of:

class Foo {
    private static value = true;
    static getValue() {
        return this.value;
    }
}

Prefer:

class Foo {
    private static value = true;
    static getValue() {
        return Foo.value;
    }
}

See https://github.com/ajafff/tslint-consistent-codestyle/blob/master/docs/no-static-this.md

Additional context

Spinning off of #2040: moving some rules of tslint-consistent-codestyle to tslint core.
Note that as ajafff mentioned, copied code without substantial changes needs to retain the original license.

@mbelsky
Copy link
Contributor

mbelsky commented Jan 21, 2019

Hey,
I'm working on this rule's implementation. Could you please assign this issue to me?
Also if the 'static-this' name isn't final please let me know what do you think about name it 'no-static-this'?

@JoshuaKGoldberg
Copy link
Contributor Author

@mbelsky there's no way to assign someone who's not in the Palantir org unfortunately. Posting here should be enough indicator to ask others not to tackle it soon, hopefully!

Re no-static-this vs static-this, is there a reason you'd prefer the name change? Prefixing rule names with no- has a couple downsides:

  • It makes them less discoverable (harder to scan through) in lists of rules
  • It discourages adding options for the opposite style - in this case, if folks would prefer the this for brevity

@onehorsetown
Copy link

onehorsetown commented Mar 13, 2019

This improperly flags this in an anonymous class returned by a static function, something that is typically used to construct higher-order components:

interface Bar {}

class Foo {
  public static higherOrderComponent(): any {
    return class implements Bar {
      public constructor() {
        console.log(this);   // "this" is valid here but tslint is complaining
      }
    };
  }
}

@mbelsky
Copy link
Contributor

mbelsky commented Mar 13, 2019

Hey,
Thanks for update. I'm going to bed now, so I'll try to fix that tomorrow if no one take it.

@mbelsky
Copy link
Contributor

mbelsky commented Mar 14, 2019

@JoshuaKGoldberg could you please create a new issue for this false positive case and assign to me?

@onehorsetown
Copy link

I opened a new issue: #4579

I also noticed above that Joshua indicated that he could not assign something to you:

#4428 (comment)

@JoshuaKGoldberg
Copy link
Contributor Author

Oh yes, this issue was resolved by the original PR, and #4579 tracks the followup false positive case. Thanks folks!

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

No branches or pull requests

3 participants