Remove rule @typescript-eslint/no-empty-function

[swansontec]

What version of this package are you using?

v14

What problem do you want to solve?

The upstream Standard.js project has access to the no-empty-function rule in plain-old ESLint, but has chosen not to activate it.

If this rule seems useful, then whoever wants to champion it should start a discussion on upstream Standard.js. If upstream adopts it, then this repo should continue to include it. If they don't adopt it, then this repo should remove it.

I realize that eslint-config-standard-with-typescript includes many rules that upstream does not, since we have access to type information. However, in cases where there is a 1:1 match between the Typescript rule and the ESLint rule, I think we should leave the decision to upstream, since it's just as valuable or harmful there as it is here.

What do you think is the correct solution to this problem?

Remove @typescript-eslint/no-empty-function.

After living with it for a while, I personally find this rule to be quite unhelpful. I write a lot of React code, where it's common to have do-nothing class methods like render() for components that don't draw to the screen. It's perfectly obvious to what is going on in these cases, and yet this rule requires me to put in an pointless comment. That is why I don't really want to open the discussion at the Standard.js level, but will leave that for others to do.

Are you willing to submit a pull request to implement this change?

Of course.

[LinusU]

Seems reasonable to me. I think the first step would be to check upstream if we can include the rule there though 🤔

Regarding render btw, according to the typings you're only allowed to return null or an element, not undefined. I've always implemented empty components as return null since otherwise TypeScript complains...

[LinusU]

I was thinking something along "surely not many projects at all have empty functions", but I was very wrong 😅

# tests 219
# pass  165
# fail  54

I think it's reasonable to disable this, as I don't see that upstream will enable it, and I think that we should follow upstream 👍

[mightyiam]

Thank you, @swansontec. Since your main example is non-render react components and since @LinusU reports that in TypeScript those should return null, could we have that clarified, first, please? If that is the main reason for the failures in standard and if those projects convert to TypeScript, then perhaps the correct solution would be to convert their empty functions to return null anyway.

In general, we follow standard, but it seems like empty functions are less tolerated by TypeScript itself. And the edge case could be a good candidate for an eslint-disable comment.

[LinusU]

I don't think we have any React projects in the Standard tests, most that I looked at wasn't React specific at least.

[...] but it seems like empty functions are less tolerated by TypeScript itself.

I'm not sure what you mean about this? It's perfectly valid to have an empty function, and TypeScript will correctly infer that function to return undefined.

You can also pass empty functions to any receiver expecting a function that returns void or undefined, and empty async functions to any receiver expecting void, Promise<void>, or Promise<undefined>.

I think that we should try and follow upstream as closely as possible ☺️

[swansontec]

Regarding render btw, according to the typings you're only allowed to return null or an element, not undefined. I've always implemented empty components as return null since otherwise TypeScript complains...

Oops, sorry for the bad example! I've been focusing on our backend recently, and I guess my memory got a little blurry. I realize this is moot, since the issue is closed, but here are two genuine examples for the record:

export const silentLogger: EdgeLogger = {
  info () {},
  warn () {},
  error () {}
}

function doNothing (): void {}

// Later:
const {
  onAddressesChecked = doNothing,
  onBalanceChanged = doNothing,
} = this.props

// Now we can use these callbacks without worry, since they always exist...

[LinusU]

For the latter, I personally use onAddressesChecked?.() when calling the function. Using optional chaining instead of setting the variable to a useless function.

But the first example is a very good one, and I think that there was a lot of that in the packages that wasn't compatible in the ecosystem ☺️

[swansontec]

For the latter, I personally use onAddressesChecked?.() when calling the function. Using optional chaining instead of setting the variable to a useless function.

I agree, that's probably a good cleanup, and best for all new code! However, I wrote this particular code in 2017, but optional chaining only came out in the latest version of Typescript.

[mightyiam]

Lint-disabling comments are a legitimate part of code. Demonstrating a legitimate false-positive case for a rule does not negate the value of the rule. It just shows an exception, in this case, that should be handled with a disable comment, if you ask me.

[LinusU]

That ~25% of the most popular packages that uses standard are using empty functions leads me to believe that this is more than an anomaly. But it would be interesting to look at the cases in depth and see how they are using it, and if there is a better way.

That said, I think that this discussion should be continued in the normal Standard repo since the rules should be enabled there first ☺️