molindo / eslint-config-molindo

ESLint config that implements the Molindo styleguide and helps to catch errors.
MIT License
1 stars 2 forks source link

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

Open amannn opened 2 years ago

amannn commented 2 years ago

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 commented 2 years ago

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 commented 4 months ago

See also: https://www.totaltypescript.com/method-shorthand-syntax-considered-harmful