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

Consider changing @typescript-eslint/method-signature-style to "property" #87

Open
amannn opened this issue Apr 15, 2022 · 2 comments
Open

Comments

@amannn
Copy link
Member

amannn commented Apr 15, 2022

These are not equivalent:

{
  // Bivariant
  methodA(event: MouseEvent<HTMLElement>): void;
  // Contravariant
  methodB: (event: MouseEvent<HTMLElement>) => void;
}

We might want to consider the second syntax to disallow covariance.

"@typescript-eslint/method-signature-style": ["error", "property"]

This example helped me understand the situation:

type Props = {
  onClick(event: MouseEvent<HTMLElement>): void;
}

function Button({onClick}: Props) {
  const Component = Math.random() < 0.5 ? 'button' : 'div';
  return (
    <Component onClick={onClick} type="button">
      Button
    </Component>
  );
}

The implementation is just a contrived example, but the point is that the click event handler needs to use the generic HTMLElement, since it can't be known at compile-time if it's going to be a HTMLButtonElement or a HTMLDivElement – therefore a common super class is used.

The problem is that a consumer can assume a potentially problematic specific type when providing an event handler:

function onButtonClick(event: MouseEvent<HTMLButtonElement>) {
  event.currentTarget.disabled; // Only exists on `HTMLButtonElement`, not on `HTMLDivElement`
}

// Ok with covariance
return <Button onClick={onButtonClick} />;

With covariance this is allowed and potentially problematic. By using the property syntax, the above assignment to onClick will be an error and you'd have to accept an HTMLElement in the event. However you can still refine it with instanceof:

function onButtonClick(event: MouseEvent<HTMLElement>) {
  if (event.currentTarget instanceof HTMLButtonElement) {
    event.currentTarget.disabled;
  }
}

return <Button onClick={onButtonClick} />;

Personally, I like the method syntax and I think it's a bit closer to the function declaration syntax that we use, that's why I picked it originally. But TypeScript doesn't seem to support a compiler option that opts-out of bivariance for the method syntax.

See also: Wikipedia on covariance and contravariance

@amannn
Copy link
Member Author

amannn commented Apr 15, 2022

Note however that TypeScript still uses bivariance in arrays which to my knowledge can't be avoided. So it's still required to be careful when working with sub-typing.

Not sure if it's worth adding this rule.

@amannn
Copy link
Member Author

amannn commented Feb 12, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant