matrix-org
Switch away from using method-defined `interface`s for base-class definition
interface ClassInterface {
callable(f: string | number): void;
}
class Thing implements ClassInterface {
public callable(f: string): void {
}
}
class OtherThing implements ClassInterface {
public callable(f: number): void {
}
}
Related to https://github.com/matrix-org/matrix-js-sdk/issues/2113, using interfaces can cause above situations, where any person going from an interface definition will possibly draw the wrong conclusions.
According to https://typescript-eslint.io/rules/method-signature-style/, it's better to change from method syntax to property syntax;
interface T1 {
func(arg: string): number;
}
type T2 = {
func(arg: boolean): void;
};
interface T3 {
func(arg: number): void;
func(arg: string): void;
func(arg: boolean): void;
}
to
interface T1 {
func: (arg: string) => number;
}
type T2 = {
func: (arg: boolean) => void;
};
// this is equivalent to the overload
interface T3 {
func: ((arg: number) => void) &
((arg: string) => void) &
((arg: boolean) => void);
}
This'll correctly error the example in the first comment.
Link is https://typescript-eslint.io/rules/method-signature-style/, to switch to a "property" based syntax, which (together with strict function type checking) enables proper checking of class implementations of interfaces.
That doesn't really explain why we should go through and make the change though - what is the proposed actionable gain?
Please look at the first comment again, and carefully observe the function parameters in the functions. Typescript does not properly type-check this as long as it is defined with a method call syntax. This means that any annotation of implements X is functionally useless, as it does not properly enforce a nuanced interface.
This will create a false sense of security for anyone who follows the signature type definitions for its application usages.
As typescript is not properly checking this, this will allow a programmer to define a class more "narrow" in scope with the parameters it's functions receive, not properly checking each type variant that comes in.
This would allow a programmer to define a not-nullable type on a parameter which is nullable in the implementing interface.
This could lead to headaches, programmer bugs, runtime nullability problems (propagated undefined and errors on calling undefined as a function), and at worst, vulnerabilities.
On a personal note, sorry that I had to explain this this verbose, but originally I wrote this issue with the idea that this highlighted discrepancy in the library spoke for itself.
Previously we've made the decision to deliberately not use this syntax, which is why I'm pushing for rationale for why we should suddenly pick it up. We valued developer friendliness and familiarity over the safety, thus made use of method definitions rather than property syntax.
I'd be interested if we can instead convince typescript (either through config or (potentially) custom tooling) to instead apply the same safety against method definitions rather than go through and rewrite a bunch of interfaces to be less human-friendly.
This is the config option, to enforce typescript to treat each property method signature as a function, instead of its constituent parts individually.
I'm not convinced custom tooling will make this any more manageable, unless interfaces are transpiled silently in the type-checker to accommodate these type of checking, which i then believe will poke a lot more moving parts therein, which could be more trouble than its worth.