feat(eslint): enable @typescript-eslint/no-unnecessary-type-parameters#112404
feat(eslint): enable @typescript-eslint/no-unnecessary-type-parameters#112404JoshuaKGoldberg merged 5 commits intomasterfrom
Conversation
| } | ||
|
|
||
| export function getEscapedKey<Value extends SelectKey | undefined>(value: Value): string { | ||
| export function getEscapedKey(value: SelectKey): string { |
There was a problem hiding this comment.
[Explanation] This is an example of a type parameter that did nothing. Value didn't change the function's return type at all. So value's type effectively already was SelectKey.
| export function useOrganizationRepositories<T extends Repository = Repository>( | ||
| {query = {}} = {} as Props | ||
| ) { | ||
| export function useOrganizationRepositories({query = {}} = {} as Props) { |
There was a problem hiding this comment.
[Explanation] This, too, is an example of a type parameter doing nothing. T shows up later in the function, but those uses don't impact the return type. https://typescript-eslint.io/rules/no-unnecessary-type-parameters/#im-using-the-type-parameter-inside-the-function-so-why-is-the-rule-reporting
|
|
||
| type KnownDataDetails = Omit<KeyValueListDataItem, 'key'> | undefined; | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters |
There was a problem hiding this comment.
[Explanation] This is used in a few places and seems to be a kind of intentional "just give me the value as a specific type". I tried for a couple minutes to fix it locally but it was nontrivial.
| threshold: 0.6, | ||
| location: 0, | ||
| distance: 100, | ||
| maxPatternLength: 32, |
There was a problem hiding this comment.
[Explanation] This is, I believe, legitimately dead code now detected by TypeScript!
Before, createFuzzySearch in fuzzySearch.tsx had a type parameter for its options. That meant any arbitrary property like maxPatternLength would be allowed and counted as part of the type argument.
Now that options is only ever Fuse.IFuseOptions<T>, unknown property names are not allowed. maxPatternLength was removed in Fuse v5. https://github.com/krisk/Fuse/blob/main/CHANGELOG.md#version-500-beta
|
|
||
| /** | ||
| * Identity function that casts the raw completion data to a typed shape. | ||
| * Identity function that asserts the raw completion data to a typed shape. |
There was a problem hiding this comment.
[Explanation] This is super nitpicky, please forgive me: but assertions and casts are not the same. This function asserts (type system only, unsafe). It doesn't cast (runtime change, safe in this context).
| return function WithDomainRedirectWrapper(props: P) { | ||
| export function withDomainRedirect(WrappedComponent: RouteComponent) { | ||
| // eslint-disable-next-line @typescript-eslint/no-restricted-types | ||
| return function WithDomainRedirectWrapper(props: object) { |
There was a problem hiding this comment.
[Explanation] Our ESLint config includes an option for @typescript-eslint/no-restricted-types copied from the old defaults for @typescript-eslint/ban-types:
Lines 657 to 661 in 2795da8
See https://typescript-eslint.io/blog/revamping-the-ban-types-rule: that old value is no longer the recommendation. Mucking with that is out of scope for this PR, I think.
| ): ChildType | null; | ||
| findChild(predicate: (child: BaseNode) => boolean): BaseNode | null; | ||
| findChild<ChildType extends BaseNode = BaseNode>( | ||
| predicate: (child: BaseNode) => child is ChildType |
There was a problem hiding this comment.
[Explanation] These methods are used in roughly two types situations, corresponding to the two function overloads in order:
- Oftentimes they're given a true type predicate. This gives us a nicely narrowed return type, so we put it first as the preferred form.
- As a fallback, if the function just returns
boolean(no type system indication as to what type the value is), we just return the same broad type as the value.
Aside: I haven't had a good excuse to write function overloads with generics in a while. It was nice to actually have a use for that knowledge, for once. 🥲
P.S. https://effectivetypescript.com/2024/02/27/type-guards is a great article on type predicates.
2795da8 to
0fb330c
Compare
0fb330c to
d590db9
Compare
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-type-parameters | ||
| export function getApiQueryData<TResponseData>( |
There was a problem hiding this comment.
love this! It surfaces type parameters that are type assertions in disguise ❤️
Co-authored-by: @TkDodo
… static/gsApp/utils/trackSpendVisibilityAnalytics.tsx
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8422c6d. Configure here.
#112404) Enables [`@typescript-eslint/no-unnecessary-type-parameters`](https://typescript-eslint.io/rules/no-unnecessary-type-parameters) internally. This enforces the ["Golden Rule of Generics"](https://effectivetypescript.com/2020/08/12/generics-golden-rule): > **Rule: If a type parameter only appears in one location, strongly reconsider if you actually need it.** Summarizing the rule's docs (would recommend reading, especially the FAQs) and that excellent blog post (definitely would recommend the read!): if a type parameter (commonly: `<T>`) is only used in one place, it's not actually useful. Single-use type parameters almost always either do nothing or mask an unsafe type assertion. I added explainer comments inline in the PR. Fixes ENG-7007
|
Fixes ENG-7256. |

Enables
@typescript-eslint/no-unnecessary-type-parametersinternally. This enforces the "Golden Rule of Generics":Summarizing the rule's docs (would recommend reading, especially the FAQs) and that excellent blog post (definitely would recommend the read!): if a type parameter (commonly:
<T>) is only used in one place, it's not actually useful. Single-use type parameters almost always either do nothing or mask an unsafe type assertion.I added explainer comments inline in the PR.
Fixes ENG-7007