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

ESLint: Enable the method-signature-style rule #2618

Closed
acicovic opened this issue Jul 9, 2024 · 4 comments · Fixed by #2664
Closed

ESLint: Enable the method-signature-style rule #2618

acicovic opened this issue Jul 9, 2024 · 4 comments · Fixed by #2664

Comments

@acicovic
Copy link
Collaborator

acicovic commented Jul 9, 2024

Is your feature request related to a problem?

The TL;DR is that we should enable the https://typescript-eslint.io/rules/method-signature-style rule in our code, and fix any violations.

To see why, this is an example (originally quoted by @sirreal, can be found here):

type AnyItem = Record<string, any>;

interface Post extends AnyItem { status: string; }

type F<T> = (item: T) => boolean;
interface IF<T> { (item: T): boolean; }

interface Base<T extends AnyItem> {
  isEligible(item: T): boolean;
  isEligible1: F<T>;
  isEligible2: IF<T>;
}

const base: Base<AnyItem> = {
  // Why is this is OK?
  isEligible: (item: Post) => item.status.startsWith('eli:'),

  // These are both type errors as expected.
  isEligible1: (item: Post) => item.status.startsWith('eli:'),
  isEligible2: (item: Post) => item.status.startsWith('eli:'),
}

Relatedly: https://x.com/mattpocockuk/status/1803753059480121593

@acicovic acicovic added this to the Summer 2024 milestone Jul 9, 2024
@sirreal
Copy link

sirreal commented Jul 9, 2024

I endorse this change 😀
The next version of wp-scripts will enable this in the recommended ruleset.

@acicovic
Copy link
Collaborator Author

acicovic commented Jul 9, 2024

Note to our future selves: As we aren't currently using the last version of wp-scripts due to WordPress/gutenberg#62202, we do want to add this rule, at least until we're able to upgrade.

@acicovic
Copy link
Collaborator Author

Related to #2624. If we become able to upgrade wp-scripts, then this rule will be auto-included. It probably makes sense to focus on #2624, as it should solve two issues at once.

@acicovic
Copy link
Collaborator Author

I finally went to fix this, since it was an easy win/change.

@acicovic acicovic modified the milestones: Summer 2024, 3.17.0 Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants