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

[explicit-function-return-type] Add option to ignore methods with specific names #541

Closed
stasgavrylov opened this issue May 20, 2019 · 10 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@stasgavrylov
Copy link

Hi everyone, and thanks for all the great work on this plugin.

It would be nice if explicit-function-return-type would allow a way to set specific functions/methods to ignore for this check.
I don't think it makes a lot of sense to declare types on React lifecycle methods, like render or componentDidMount.
These methods probably shouldn't be ignored by eslint-plugin itself, but maybe it could provide an escape hatch for other plugins to use?

Right now the only 2 options available for this rule still do not allow to do this.

Thanks.

@stasgavrylov stasgavrylov added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels May 20, 2019
@bradzacher bradzacher changed the title [explicit-function-return-type] Allow to omit return type for React lifecycle methods [explicit-function-return-type] Add option to ignore methods with specific names May 20, 2019
@bradzacher bradzacher added enhancement: plugin rule option New rule option for an existing eslint-plugin rule and removed triage Waiting for maintainers to take a look labels May 20, 2019
@golopot
Copy link
Contributor

golopot commented Jun 16, 2019

What should the option be like? How about

{
  ignoreMethods: [{
    superClass: 'React.Component',
    method: 'render',
  }]
}

?

@bradzacher
Copy link
Member

bradzacher commented Jun 17, 2019

TBH the more I think about it the less I like it.
The checks to decide if a function should be ignored based on a filter are either very loose and far reaching (node.name === config.name), or they're complex to implement, and rather specific (like checking the function is a method and belonging to a class with superclass that is specifically React.Component - what about if someone import { Component } from 'react'?)

I don't think there's any good way to implement a "filter" config that works with the spirit of the rule (which is ensuring that every function has a defined return type).


The better way to implement this would be to add an option to enable "superclass" checks for class methods.
i.e. similar to the typed function check - ignore the function if there exists a function of the same name on the super class OR interface.
Because this check would use the resolved type of the class/interface, it wouldn't need configuration that's pretty unsafe.

@golopot
Copy link
Contributor

golopot commented Jul 9, 2019

How about an option like:

{ ignoreFunctionNames: ['render', 'componentDidMount', ...etc]}

Sure it is an overkill that affects much more functions than is intended. But it has the benefit that it is dead simple.

@bradzacher
Copy link
Member

bradzacher commented Jul 9, 2019

I mean I don't hugely understand why it's such a burden to write out the 5-6 characters that is : void - all of the react lifecycle methods in question return void (except render, which has many different possible return types, the most common of which is React.ReactNode).

Also the future of React is functional components, which need a type annotation anyways. So I don't hugely like the idea of adding an option purely to support an old way of doing things.


Like I said - I don't like having a whitelist of function names because it's indiscriminate of the context, and is also a pain to implement properly because you have to handle variable declarations, class methods, class properties, function declarations, arguments, etc. There's a lot of cases for such a simple option.

@SinrVolcano

This comment has been minimized.

@sonsina
Copy link

sonsina commented Oct 29, 2020

Hi @bradzacher, wanted to know if there's an update here. I think that usually it's a non issue but when having to type the complex type in mapStateToProps or mapDispatchToProps, things look ugly and more copy paste then wanted.

I'd appreciate you're opinion here.

@benjamincharity
Copy link

Chiming in here. This was a request at my current company also. I was hoping it supported ignoredMethodNames like @typescript-eslint/explicit-member-accessibility does.

https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/explicit-member-accessibility.md

@pixelbucket-dev
Copy link

I have the exact same issue: I started using explicit-module-boundary-types which has a nice option allowedNames which does exactly this. However, later I realised it does not enforce signature types on private methods.

Therefore, I switched to rule explicit-function-return-type now realising that all methods are required a return type but above filter does not work. This adds unnecessary burden to type internal life cycle methods (StencilJS) or private render methods (they always return some form of VDOM or VDOM[]).

A allowedNames rule here would be very nice. Even better if that could be extended to support glob pattern, e.g. filter all methods/functions that start with "render" ("render*").

@neelance
Copy link

We currently use a patch for typescript-eslint that does the same for functions that start with "setup" ("setup*").

@JoshuaKGoldberg JoshuaKGoldberg added the accepting prs Go ahead, send a pull request that resolves this issue label Oct 25, 2021
@armano2
Copy link
Member

armano2 commented May 31, 2022

closing as explicit-function-return-type rule has now support for allowedNames https://typescript-eslint.io/rules/explicit-function-return-type/#allowednames

@armano2 armano2 closed this as completed May 31, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

10 participants