class-methods-use-this documentation is unclear about how to correct the problems

[fega]

What rule do you want to change?

class-methods-use-this

Does this change cause the rule to produce more or fewer warnings?

fewer warning

Please provide some example code that this change will affect:

In javascript we cannot call static methods from non-static methods, ie:

/*eslint class-methods-use-this: "error"*/
/*eslint-env es6*/
> class A {
 m(){console.log("hi")} // this shouln't warn
 n(){this.m()}
}
[Function: A]
> let a = new A()
undefined
> a.n()

because if I change it...

> class A {
 static m(){console.log("hi")}
 n(){this.m()}
}
[Function: A]
> let a = new A()
undefined
> a.n()
TypeError: this.m is not a function
    at A.n (repl:3:10)
    at repl:1:3
    at sigintHandlersWrap (vm.js:22:35)
    at sigintHandlersWrap (vm.js:73:12)
    at ContextifyScript.Script.runInThisContext (vm.js:21:12)
    at REPLServer.defaultEval (repl.js:340:29)
    at bound (domain.js:280:14)
    at REPLServer.runBound [as eval] (domain.js:293:12)
    at REPLServer.<anonymous> (repl.js:538:10)
    at emitOne (events.js:101:20)

I'm aware that as mdn states, we can call using the className or this.constructor, but this structure looks for me weird and verbose.

How will the change be implemented? (New option, new default behavior, etc.)?

I propose that this Rule should have an option to disable it if the method is being called from another method.
😄

[eslintbot]

Hi @fega, thanks for the issue. It looks like there's not enough information for us to know how to help you.

If you're reporting a bug, please be sure to include:

1. The version of ESLint you are using (run eslint -v)
2. What you did (the source code and ESLint configuration)
3. The actual ESLint output complete with numbers
4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

[ljharb]

In this case, your code is incorrect - n() { this.m(); } won't work, and will produce a runtime error.

Your only options here are n() { this.constructor.m(); } (which is not a good idea, because the constructor property isn't reliable) - or, n() { A.m(); } - in other words, hardcode the class name (which is by far the better choice).

This also is a runtime error, and not an issue with eslint, so I'm not sure what rule modification would help you here.

[fega]

@ljharb yeah, this doesn't work because is an static method. but the rule 'class-methods-use-this' is warning if I don't use the keyword 'static' in the method m...

My point is that eslint shouldn't be warning in the following case:

/*eslint class-methods-use-this: "error"*/
/*eslint-env es6*/
> class A {
 m(){console.log("hi")} // this shouln't warn
 n(){this.m()}
}
[Function: A]
> let a = new A()
undefined
> a.n()

Is in that way more clear?

[ljharb]

@fega Yes, much more clear, thanks!

In that case, that's the sole purpose of the rule - to warn when it needn't be a class method. You could do it this way, and both satisfy the rule and minimize your class's API surface:

function m() {
  console.log("hi");
}
class A {
 n(){m()}
}

[fega]

@ljharb yeah, but this is only an example of a warning case, but sometimes is necessary maintain the m() inside the class,like cohesion in the API or maybe you want to keep the m() when the you write something like class B extends A

Since I'm using and refactoring a code base with a large use of classes, I've had a lot of awful surprises due this behavior

[platinumazure]

@fega As @ljharb noted, this rule is slightly opinionated and encourages you to reduce your API surface by avoiding exposing instance methods that don't use this.

For this rule (and any other ESLint rule), we recognize that there are times where a rule does not apply exactly 100% of the time. In those cases where a rule configuration option does not exist to control when the rule should or should not be enforced, you can always use // eslint-disable-line comments (or similar) to avoid ESLint reporting an error in specific cases where violating the rule is part of your design. So you could use one of those comments in this case if you believe the rule does not need to be followed in your particular case.

[fega]

@platinumazure well, I don't see why should be a problem give to the rule a little bit more flexibility.

For the other side, I made some mistakes refactoring due this rigid rule behavior (that, I think, shouldn't happen when you use a linter).

Also, I'm using those kinds of comments, but you know, that an overpopulation of comments adds more noise in the code and could leads into forget to refactor some methods that really could need changed in order to reduce the API surface.

Another point, is that the rule is not encouraging to reduce the API surface, is encouraging to add the "static" keyword, that could introduce a lot of unexpected problems when this method is being used in others parts of the code.

[not-an-aardvark]

For the other side, I made some mistakes refactoring due this rigid rule behavior (that, I think, shouldn't happen when you use a linter).

I think you might be misunderstanding the purpose of the rule. It's not trying to say that you should simply be able to add the static keyword and have everything work -- instead, it's encouraging you to reconsider your API design, which would necessarily involve changing other parts of the code. It seems like what you're proposing would go against the purpose of the rule, so you might be better off by just not enabling the rule in the first place.

[ljharb]

Sticking static in front of a method will break things just like sticking a * or async in front of it. It's not the linter's job to prevent you from misunderstanding a feature unrelated to the rule in question :-/

[fega]

@not-an-aardvark, @ljharb, maybe the problem was in the documentation, it states

"If a class method does not use this, it can safely be made a static function."

That's not true for all of the use cases.
And makes me think that the problem will be solved simply adding the static keyword. @

[not-an-aardvark]

I see what you mean. Maybe that should be reworded to avoid implying that you can add static without doing anything else.

[ljharb]

Indeed; that documentation should be improved.