[TypeScript] Should reactHelpers functions be Type Guard or not?

[SSW-SCIENTIFIC]

I have written typings to reactHelpers (#9565) but these functions (isMuiComponent and isMuiElement) are used for type checking.
So I think it is better to declare these functions as type guard.

By implementation,

export function isMuiElement(element: any, muiNames: Array<string>)
    : element is ReactElement<P> & { type: { muiName: string } };
export function isMuiComponent<P>(element: any, muiNames: Array<string>): element is { muiName: string };

is considerable declaration (of course it looks not simple, so not good).

Is there any good type declaration for MUI element/component? or already exist? or the above functions should not be type guard (keep current, simply return boolean)?

[pelotom]

Seems like a good idea to me, would you like to open a PR?

To be completely accurate I think maybe we want to use more specific custom types, i.e.

type NamedMuiComponent = React.ComponentType<{}> & { muiName: string };

interface NamedMuiElement {
  type: NamedMuiComponent;
  props: StandardProps<{}, never>;
  key: string | number | null;
}

export function isMuiElement(element: any, muiNames: string[]): element is NamedMuiElement;
export function isMuiComponent(element: any, muiNames: string[]): element is NamedMuiComponent;

About muiName... I notice looking through the code that some components set this, e.g. Select, and others don't, e.g. Button. So it looks like maybe it should be { muiName?: string }? Or is this inconsistency a bug? Also, what's the relationship (if any) between muiName and the name option passed to withStyles? /cc @oliviertassinari

[oliviertassinari]

what's the relationship (if any) between muiName and the name option passed to withStyles?

@pelotom Non, they solve independent problems.

Or is this inconsistency a bug?

muiName is only defined for rendering based logic. If we don't need it, we don't provide it.
This is consistent but not symmetric.

[pelotom]

Ok, so then it should be { muiName?: string }. I've corrected the types I gave above.

[pelotom]

Oh! But obviously if you found it to have a certain name, then it definitely has that name. So it should not be optional. Re-corrected 😄

[pelotom]

In light of that, we might want to call these custom types NamedMuiComponent / NamedMuiElement just to be clear that these functions only apply to named components/elements.

[SSW-SCIENTIFIC]

Thank you for opinions!

@pelotom
I am not familier with React on TypeScript, so I want to know about differences between the above and type NamedMuiComponent<P = {}> = React.ComponentType<P> & { muiName: string };

/* I think annotations/descriptions for generic parameters in .d.ts files are not developer friendly... */

[pelotom]

@SSW-SCIENTIFIC the main difference is the lack of a P type parameter. There's no point in giving a type parameter to NamedMuiComponent since it is only used by these functions, and they can't provide any more specific types for <P>. I take back the use of never, it should be {}.

[SSW-SCIENTIFIC]

@pelotom
Thank you for description!

In my use case, type parameter P is useful.
For example,

if (isMuiElement(element, ['ListItem'])) {
    element.props.onClick && element.props.onClick();
    // this is invalid because the type of element.props is StandardProps<{}, never, never>
    // and it does not have onClick property
}

IMHO, this code should work.

I know that isMuiElement checks only validity as ReactElement and muiName property by its implementation, no checks for props, however, React's isValidElement is same situation and @types/react#L265 shows isValidElement has type parameter P to specify the type of props:

function isValidElement<P>(object: {} | null | undefined): object is ReactElement<P>;

So I now propose the declaration below:

type NamedMuiComponent<P> = React.ComponentType<P> & { muiName: string };
type NamedMuiElement<P> = React.ReactElement<P> & {
    type: { muiName: string };
}

export function isMuiElement<P>(element: any, muiNames: string[]): element is NamedMuiElement<P>;
export function isMuiComponent<P>(element: any, muiNames: string[]): element is NamedMuiComponent<P>;

or NamedMuiElement can be

interface NamedMuiElement<P> extends React.ReactElement<P> {
    type: (string | React.ComponentClass<P> | React.SFC<P>) & { muiName: string };
}

or

interface NamedMuiElement<P> {
    type: NamedMuiComponent<P>;
    props: P;
    key: React.Key | null;
}

All of above three declaration make the code below valid:

if (isMuiElement<ListItemProps>(element, ['ListItem'])) {
    element.props.onClick && element.props.onClick();
}

I do not know which is the best...
And are there any considerable trouble on these declarations?

[pelotom]

In my use case, type parameter P is useful.

Yes, it's useful, but it's not safe 🙂 . I know React's isValidElement has this type, but it's wrong IMO. Consider, you want this code to work:

if (isMuiElement<ListItemProps>(element, ['ListItem'])) {
    element.props.onClick && element.props.onClick();
}

but if this worked, you could just as easily do

if (isMuiElement<{ bogus(): void }>(element, ['ListItem'])) {
    element.props.bogus();
}

So with this typing all you're really doing is an unsafe cast, and you might as well be explicit about that:

if (isMuiElement(element, ['ListItem'])) {
    const { onClick } = element.props as ListItemProps
    if (onClick) onClick();
}

It's not that much extra code, and it alerts the reader of your code that a downcast is happening, rather than hiding that fact.

[SSW-SCIENTIFIC]

@pelotom
Ok I understood, and open PR without type parameter soon.