[DISCUSS] Typescript: EuiContextMenuItem icon should be able to be a non-react element

[stacey-gammon]

I'm adding functionality that will allow plugins to add menu items. I'm specifying the icon type as Element | JSX.Element;. This conflicts with the icon type in eui:

export type EuiContextMenuItemIcon = ReactElement<any> | string;

We don't want to force plugins to write their code in react, so I think this type should be more generalized. Open for discussion though.

[stacey-gammon]

Using Element | JSX.Element | string fixes the issue:

But I'm not sure that is the right fix. Icon is used in React.cloneElement which has a slew of input types, but all are more specific than Element and JSX.Element:

    // DOM Elements
    // ReactHTMLElement
    function cloneElement<P extends HTMLAttributes<T>, T extends HTMLElement>(
        element: DetailedReactHTMLElement<P, T>,
        props?: P,
        ...children: ReactNode[]): DetailedReactHTMLElement<P, T>;
    // ReactHTMLElement, less specific
    function cloneElement<P extends HTMLAttributes<T>, T extends HTMLElement>(
        element: ReactHTMLElement<T>,
        props?: P,
        ...children: ReactNode[]): ReactHTMLElement<T>;
    // SVGElement
    function cloneElement<P extends SVGAttributes<T>, T extends SVGElement>(
        element: ReactSVGElement,
        props?: P,
        ...children: ReactNode[]): ReactSVGElement;
    // DOM Element (has to be the last, because type checking stops at first overload that fits)
    function cloneElement<P extends DOMAttributes<T>, T extends Element>(
        element: DOMElement<P, T>,
        props?: DOMAttributes<T> & P,
        ...children: ReactNode[]): DOMElement<P, T>;

    // Custom components
    function cloneElement<P>(
        element: SFCElement<P>,
        props?: Partial<P> & Attributes,
        ...children: ReactNode[]): SFCElement<P>;
    function cloneElement<P, T extends Component<P, ComponentState>>(
        element: CElement<P, T>,
        props?: Partial<P> & ClassAttributes<T>,
        ...children: ReactNode[]): CElement<P, T>;
    function cloneElement<P>(
        element: ReactElement<P>,
        props?: Partial<P> & Attributes,
        ...children: ReactNode[]): ReactElement<P>;

So maybe I need to adjust my code to support HTMLElement or SVGElement or ReactElement.

[stacey-gammon]

cc @chandlerprall @cjcenizal and @timroes too - since this might affect you with typings and plugins and needing to support non-react plugin code.

[stacey-gammon]

I was chatting with @timroes and it sounds like React.ReactNode is the right type to use, because it can be an array, an HTMLElement, a string, or a react element.

[chandlerprall]

From @types/react, the ReactNode is ReactChild | ReactFragment | ReactPortal | string | number | boolean | null | undefined; which doesn't appear to include HTMLElement. The render() function's return type is ReactNode, and those can't return HTMLElements. Also, PropTypes.node does not allow HTMLElements.

Do you have an example of that type working for your needs?

[stacey-gammon]

Maybe because HTMLElement is a subset of one of those? It does seem to work for me:

modified eui typings:

 export type EuiContextMenuItemIcon = ReactNode;

  export interface EuiContextMenuItemProps extends CommonProps {
    icon?: EuiContextMenuItemIcon;
    hasPanel?: boolean;
    buttonRef?: RefCallback<HTMLButtonElement>;
    disabled?: boolean;
    onClick?: () => void;
  }

see no error even with action.icon being tagged as HTMLElement.

[chandlerprall]

I can't let the universe not make sense, so digging in... :)

ReactNode includes ReactFragment which includes {} in its union. This type apparently allows anything (EDIT: apparently everything except null/undefined). I setup a quick codesandbox demonstrating. All of these pass typescript's compiler:

import * as React from "react";

function foo(x: {}) {}

foo(5);
foo({});
foo({a: 'b', c: 3});
foo(NaN);
foo(<div>Hello World</div>);
foo(document.createElement('div'));

[stacey-gammon]

haha. hmmm. well, it'd unblock me? :) So maybe for the time being we could do that in EUI until we figure out a better solution?

[chandlerprall]

That aside, the component(s) where we want to allow HTML elements to be passed need to be modified to allow it. It makes sense in these cases to have a specific union type, e.g. ReactElement<any> | string | HTMLElement

[stacey-gammon]

That'll work for me!