Initial commit of typescript type definitions for EUI components & services #252

uboness · 2017-12-28T02:09:51Z

relates to #256

weltenwort

This is a great start 👍 There are some aspects which we might want to make a conscious decision about though:

component type definitions

There is an alternative to declaring classes with empty bodies. Instead one could write

export const EuiButton: React.SFC<EuiButtonProps>;

The differences I can see are the following:

it does not define a new type, it just assigns a type to the export
it does not suggest that the component as a state of type {}
it does not tempt the author to put something into the class body, but instead restricts him to declaring the prop types

I have not arrived at a conclusion about which style is better. The const style appeals to me due to its simplicity though.

inline unions

In some places the prop types are defined and exported separately (e.g. ButtonColor) and in some places they are defined inline in the props interface. Do we want to stick to one style or set some rule about which style to apply when?

common props

There are some props like className, aria-label or data-test-subject, which most components have to support. So we might want to deduplicate them and mix them into the prop type definition using intersection types:

interface CommonProps {
  className?: string;
  'aria-label'?: string;
  'data-test-subj'?: string;
}

interface AnyProps {
  [prop: string]: any;
}

interface EuiButtonProps {
  /* ... */
}

const EuiButton: React.SFC<
  CommonProps &
    AnyProps &
    EuiButtonProps
>;

pass-through props

If we know that the "rest" of the props are passed through to a specific child component, we could reflect that in the type to avoid unchecked props and missing autocompletion:

export const EuiButton: React.SFC<
  CommonProps &
    AnyProps &
    React.ButtonHTMLAttributes<HTMLButtonElement> &
    EuiButtonProps
>;

uboness · 2018-01-02T14:31:22Z

component type definitions

I wan't aware of React.SFC<P>... this seems cleaner to me

inline unions

I prefer exporting props separately (it's more open for reuse and extensions)

common props & pass-through props

++ to both

weltenwort

Another idea we might want to consider is avoiding the repitition of the ambient module definition in every .d.ts file. Instead using normal export chain in those files should work too (at least it did when I last tested):

// in src/components/badge/index.d.ts
export const EuiBadge: React.SFC<EuiBadgeProps>;

// in src/components/index.d.ts
export * from './components/badge';

// in src/index.d.ts
export * from './components';

uboness · 2018-01-02T23:57:55Z

@weltenwort updated based on your proposals. I tried to keep it as close as possible to the props that are supported in the js files... I find them to be quite inconsistent when it comes to the ...rest usage (it's not always clear what elements these will be applied to)... no the less, I just followed the code.

Another idea we might want to consider is avoiding the repitition of the ambient module definition in every .d.ts file. Instead using normal export chain in those files should work too (at least it did when I last tested):

strange... I've never seen this sort of import working within type definitions. I'm inclined to keep using the triple slash directive as this (as far as I know) is the official way you should direct the compiler to other type definitions. Aside from that, personally I don't mind the repetition of the module declaration - the nice side effect is that when you jump to it and you look at the file, you immediately know the module it belongs to.

weltenwort

I had a closer look at the code and checked it with the compiler. Aside from the individual comments below a few aspects apply to multiple files:

The compiler complains about imports inside the module augmentations, which can be easily fixed by moving them to the outer scope.
How about using React.HTMLAttributes instead of React.DOMAttributes? They provide a more complete set of properties.
Do we maybe want to apply prettier to the files (similar to the logging and new platform codebase)?

I can prepare a PR to this PR with those changes if you prefer.

weltenwort · 2018-01-03T11:20:23Z

src/components/button/index.d.ts

+
+declare module '@elastic/eui' {
+
+  import { SFC, ButtonHTMLAttributes, MouseEventHandler } from 'react';


This shows an error for me (imports in module augmentations not allowed). How about moving this to line 3?

weltenwort · 2018-01-03T11:21:26Z

src/components/button/index.d.ts

+
+  export interface EuiButtonProps {
+    onClick: MouseEventHandler<HTMLButtonElement>; //overriding DOMAttributes to make this required
+    iconType?: IconType,


A /// <reference path="../icon/index.d.ts" /> at the top is required for this to resolve correctly.

weltenwort · 2018-01-03T11:22:33Z

src/components/button/index.d.ts

+    CommonProps,
+    ButtonHTMLAttributes<HTMLButtonElement>,
+    EuiButtonEmptyProps
+    >;


You probably meant to use & instead of , here.

weltenwort · 2018-01-03T12:05:28Z

src/components/context_menu/index.d.ts

+
+  export type EuiContextMenuPanel = SFC<
+    CommonProps &
+    Omit<DOMAttributes<HTMLDivElement>, 'ref', 'onKeyDown', 'tabIndex', 'onAnimationEnd'> &


How about we use HTMLAttributes a more complete interface?

Apart from that, the second argument to Omit should be a union type, i.e. 'ref' | 'onKeyDown' | 'tabIndex' | 'onAnimationEnd'.

weltenwort · 2018-01-03T12:05:47Z

src/components/context_menu/index.d.ts

+
+  export type EuiContextMenuItem = SFC<
+    CommonProps &
+    Omit<ButtonHTMLAttributes<HTMLButtonElement>, 'type', 'ref'> &


Same as above (| instead of ,) and the ButtonHTMLAttributes don't include ref in the first place.

weltenwort · 2018-01-03T12:36:44Z

src/components/pagination/index.d.ts

+
+  export type EuiPaginationButton = SFC<
+    CommonProps &
+    Omit<EuiButtonEmptyProps, 'size', 'color'> &


a union type is required here as well

weltenwort · 2018-01-03T14:20:16Z

src/components/panel/index.d.ts

+
+  export type EuiPanel = SFC<
+    CommonProps &
+    Omit<DOMAttributes<HTMLDivElement>, 'ref'> &


ref is not included in DOMAttributes (or HTMLAttributes)

weltenwort · 2018-01-03T15:05:23Z

src/components/popover/index.d.ts

+    ownFocus?: boolean,
+    anchorPosition?: PopoverAnchorPosition,
+    panelClassName?: string,
+    panelPaddingSize?: PanelPaddingSize,


This requires a

/// <reference path="../panel/index.d.ts" />

at the top.

weltenwort · 2018-01-03T15:12:56Z

src/components/table/index.d.ts

+   */
+
+  export interface EuiTableHeaderButtonProps {
+    iconType?: IconType


This requires

/// <reference path="../icon/index.d.ts" />

weltenwort · 2018-01-03T15:13:30Z

src/components/table/index.d.ts

+  export type EuiTableRowCellCheckbox = SFC<
+    CommonProps &
+    TdHTMLAttributes<HTMLTableCellElement> &
+    EuiTableRowCellCheckbox


This should probably mean EuiTableRowCellCheckboxProps.

weltenwort

I just noticed that the components are only exported as types, but not values. Instead of the export type EuiButton = SFC<...> we need export const EuiButton: SFC<...> so they are actually defined in the value scope.

uboness · 2018-01-03T20:04:34Z

I just noticed that the components are only exported as types, but not values. Instead of the export type EuiButton = SFC<...> we need export const EuiButton: SFC<...> so they are actually defined in the value scope.

good catch... I guess that was implicit with the class exports before... will fix that

uboness · 2018-01-03T20:25:01Z

@weltenwort updated

weltenwort

Just a few more points I ran into while trying to use these definitions.

weltenwort · 2018-01-04T14:14:28Z

src/components/popover/index.d.ts

+  export const EuiPopover: SFC<
+    CommonProps &
+    HTMLAttributes<HTMLDivElement> &
+    EuiPanelProps


This should probably be EuiPopoverProps.

weltenwort · 2018-01-04T14:19:47Z

src/components/context_menu/index.d.ts

+  export type EuiContextMenuPanelId = string | number;
+
+  export interface EuiContextMenuProps {
+    panels?: EuiContextMenuPanel[],


I ran into problems when trying to use this. Turns out the panels data structure here is not the React component, but an array of objects describing the panels. I wrote a working definition for the KUI context menu a while back, which should still be compatible:

interface ContextMenuPanelItem extends React.ButtonHTMLAttributes<HTMLButtonElement> { name: string; icon?: React.ReactNode; panel?: string | number; } interface ContextMenuPanel { id: string | number; title: string; items?: ContextMenuPanelItem[]; content?: React.ReactNode; } export const KuiContextMenu: React.SFC< CommonProps & AnyProps & { initialPanelId: string | number; panels?: ContextMenuPanel[]; } >;

pfff... yea.. can you check these ones:

export type EuiContextMenuPanelId = string | number; interface EuiContextMenuPanelItem extends EuiContextMenuItemProps { name: string; panel?: EuiContextMenuPanelId; } interface EuiContextMenuPanelDescriptor { id: EuiContextMenuPanelId, title: string; items?: EuiContextMenuPanelItem[]; content?: React.ReactNode; } export interface EuiContextMenuProps { panels?: EuiContextMenuPanelDescriptor[], initialPanelId?: EuiContextMenuPanelId } export const EuiContextMenu: SFC< Omit<HTMLAttributes<HTMLDivElement>, 'className' | 'style'> & EuiContextMenuProps >;

Yes, that seems to work. Although I don't think it is correct for EuiContextMenuPanelItem (which could be called EuiContextMenuItemDescriptor to clarify the symmetry) to extend EuiContextMenuItemProps. It should not contain the hasPanel and onClick properties, for example (see

eui/src/components/context_menu/context_menu.js

Lines 170 to 198 in 84dadc5

const {

panel,

name,

icon,

onClick,

...rest

} = item;

const onClickHandler = panel

? () => {

// This component is commonly wrapped in a EuiOutsideClickDetector, which means we'll

// need to wait for that logic to complete before re-rendering the DOM via showPanel.

window.requestAnimationFrame(() => {

if (onClick) onClick();

this.showNextPanel(index);

});

} : onClick;

return (

<EuiContextMenuItem

key={name}

icon={icon}

onClick={onClickHandler}

hasPanel={Boolean(panel)}

{...rest}

>

{name}

</EuiContextMenuItem>

);

).

no fully accurate... the onClick is actually an optional prop on the descriptor as well (just like it's optional on the item itself). The only difference here is the hasPanel vs. panel. The ...rest` are passthrough props to the item itself as well. I think extending it makes sense to some degree, with perhaps a small modification:

export type EuiContextMenuPanelItemDescriptor = Omit<EuiContextMenuItemProps, 'hasPanel'> & { name: string; panel?: EuiContextMenuPanelId; };

wdyt?

true... I guess what threw me off with the onClick is the fact that

eui/src/components/context_menu/context_menu.js

Line 183 in 84dadc5

if (onClick) onClick();

changes the signature from (event: MouseEvent<T>) => void to () => void. But that's a bug with the original js file. I'll create a PR for that.

The PR is #265.

weltenwort

I think this is almost ready. Only found a few things remain (see comments below). I couldn't test every prop of every component, so I expect some detail adjustments might be necessary when the definitions are used more comprehensively.

Finally, how do we want to handle the formatting? I would be in favor of applying prettier to all definition files to remove any ambiguity.

weltenwort · 2018-01-09T10:13:10Z

src/components/context_menu/index.d.ts

+   * @see './context_menu_panel.js`
+   */
+
+  import { HTMLAttributes } from 'react';


This still throws an error for me. Could probably be removed because line 3 already contains it.

weltenwort · 2018-01-09T10:14:35Z

src/components/button/index.d.ts

+  export type ButtonSize = 's' | 'l';
+
+  export interface EuiButtonProps {
+    onClick: MouseEventHandler<HTMLButtonElement>; //overriding DOMAttributes to make this required


Should the comment refer to ButtonHTMLAttributes?

it's actually defined in DOMAttributes

weltenwort · 2018-01-09T10:16:53Z

src/components/button/index.d.ts

+  export type ButtonIconColor = 'primary' | 'danger' | 'disabled' | 'ghost' | 'text';
+
+  export interface EuiButtonIconProps {
+    onClick: MouseEventHandler<HTMLButtonElement>; //overriding DOMAttributes to make this required


Should the comment refer to ButtonHTMLAttributes?

uboness · 2018-01-09T17:35:30Z

@weltenwort updated... left the comments as they are (they seem to be very logical to me)

weltenwort · 2018-01-09T18:07:15Z

What do you think about the formatting I proposed?

uboness · 2018-01-09T22:33:28Z

What do you think about the formatting I proposed?

I think it makes sense to keep all our TS code consistent across the projects.... unfortunately, I think I'll have to ask you to do that though as my knowledge of build tools and such converges to zero. We can also do it in a separate PR

weltenwort · 2018-01-10T10:22:42Z

I updated this PR with the new formatting.

…rvices

- introduced `CommonProps` - introduced `AnyProps` - introduced `NoArgCallback<T>` - introduced `RefCallback<T>` - introduced utility types `Diff` and `Omit` (TS's counterpart for `Pick`)

- changed all component exports to value exports - changed to use HTMLAttributes instead of DOMAttributes - added missing reference directives - moved all imports to sit outside of the module declaration - fixed use of `,` instead of `|` and `&`

- fixed the context menu definitions to better reflect the props contract

- fixed misplaced import

uboness requested a review from weltenwort December 28, 2017 02:09

weltenwort reviewed Jan 2, 2018

View reviewed changes

weltenwort reviewed Jan 3, 2018

View reviewed changes

weltenwort reviewed Jan 4, 2018

View reviewed changes

weltenwort reviewed Jan 9, 2018

View reviewed changes

weltenwort approved these changes Jan 10, 2018

View reviewed changes

uboness and others added 7 commits January 10, 2018 18:39

Initial commit of typescript type definitions for EUI components & se…

324618a

…rvices

Updated all component TS type definitions to use React.SFC

3c383bf

- introduced `CommonProps` - introduced `AnyProps` - introduced `NoArgCallback<T>` - introduced `RefCallback<T>` - introduced utility types `Diff` and `Omit` (TS's counterpart for `Pick`)

applied review feedback

a90098a

- changed all component exports to value exports - changed to use HTMLAttributes instead of DOMAttributes - added missing reference directives - moved all imports to sit outside of the module declaration - fixed use of `,` instead of `|` and `&`

applied review feedback

56aea13

- fixed the context menu definitions to better reflect the props contract

applied review feedback

d0346f0

- fixed misplaced import

Reformat the .d.ts files using prettier

112c351

Updated the change log

0e59359

uboness merged commit 15f3505 into elastic:master Jan 10, 2018

uboness mentioned this pull request Jan 10, 2018

Add Typescript type definitions for all components and services #256

Closed

snide added the typescript label Apr 17, 2020


		declare module '@elastic/eui' {

		import { SFC, ButtonHTMLAttributes, MouseEventHandler } from 'react';

	const {
	panel,
	name,
	icon,
	onClick,
	...rest
	} = item;

	const onClickHandler = panel
	? () => {
	// This component is commonly wrapped in a EuiOutsideClickDetector, which means we'll
	// need to wait for that logic to complete before re-rendering the DOM via showPanel.
	window.requestAnimationFrame(() => {
	if (onClick) onClick();
	this.showNextPanel(index);
	});
	} : onClick;

	return (
	<EuiContextMenuItem
	key={name}
	icon={icon}
	onClick={onClickHandler}
	hasPanel={Boolean(panel)}
	{...rest}
	>
	{name}
	</EuiContextMenuItem>
	);

Initial commit of typescript type definitions for EUI components & services #252

Initial commit of typescript type definitions for EUI components & services #252

Conversation

uboness commented Dec 28, 2017 • edited by weltenwort Loading

weltenwort left a comment • edited Loading

Choose a reason for hiding this comment

uboness commented Jan 2, 2018

weltenwort left a comment • edited Loading

Choose a reason for hiding this comment

uboness commented Jan 2, 2018

weltenwort left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

uboness commented Jan 3, 2018

uboness commented Jan 3, 2018

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltenwort left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uboness commented Jan 9, 2018

weltenwort commented Jan 9, 2018

uboness commented Jan 9, 2018

weltenwort commented Jan 10, 2018

uboness commented Dec 28, 2017 •

edited by weltenwort

Loading

weltenwort left a comment •

edited

Loading

weltenwort left a comment •

edited

Loading

weltenwort left a comment •

edited

Loading