Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Typescript type definitions for all components and services #256

Closed
uboness opened this issue Jan 3, 2018 · 18 comments
Closed

Add Typescript type definitions for all components and services #256

uboness opened this issue Jan 3, 2018 · 18 comments
Assignees
@uboness @weltenwort
Labels
meta typescript

Comments

@uboness
Copy link
Contributor

uboness commented Jan 3, 2018 •
edited by amguerrero
Loading

We use this issue to keep track of the components and services that require Typescript type definitions.

Components

Services
@uboness uboness added the meta label Jan 3, 2018
@uboness uboness mentioned this issue Jan 3, 2018
Merged
@pugnascotia
Copy link
Contributor

I started some work to write Flow library definitions for EUI. Would that be of any interest too?

@weltenwort
Copy link
Member

In general, sure. But putting the burden on the EUI devs to keep both up-to-date might be inappropriate. 🤔

My expectation was that we would at some point completely convert EUI to typescript.

@pugnascotia
Copy link
Contributor

I did wonder. We're using Flow in Cloud UI, so I'd half-expected we'd keep the libdefs there.

@weltenwort
Copy link
Member

Hm, I did not know about that. But given that they have to be maintained anyway, they could just as well live here. I know the feature sets of the two languages are not equivalent, but maybe there is some way to automate this for a subset of features?

@weltenwort
Copy link
Member

There is https://github.com/joarwilk/flowgen, but the longer I think about it, the less convinced I am that automation can produce sufficient quality ;)

@pugnascotia
Copy link
Contributor

From the bit of work I've done so far, the types were pretty easy to write, so it might not be so onerous to have both. The risk is that they'd drift, but maybe at some point we could have some automation to check that both TS and Flow definitions change in sync. I'm just thinking out loud.

@weltenwort
Copy link
Member

I agree, writing the type definitions is not hard. Making small adjustments for maintenance should be even easier. Noticing that these adjustments are necessary seems to be the biggest challenge.

@uboness
Copy link
Contributor Author

uboness commented Jan 11, 2018

I see no reason not to have flow type definitions here as well

@snide
Copy link
Contributor

snide commented Mar 9, 2018

@epixa Can you chime in on here to let us know where typescript support should fit in the priorities for EUI. It's not something @cjcenizal, @cchaos or I consider day to day and if it's a priority for the organization I wanna make sure we have a clear owner of responsibility here. Right now it's pretty much just being done by @uboness and @weltenwort as they need it.

@epixa
Copy link

epixa commented Mar 10, 2018

Let's separate short and long term for a moment:

Short term: From a tactical standpoint, the absolute priority right now for EUI should be rapidly building out the missing components, and it seems that is best accomplished using native ecmascript for now. I think it's great that @uboness and @weltenwort are building out typescript definitions in parallel because that will be really helpful to Kibana down the line, but if it is slowing us down in rolling out EUI more extensively, we should shut it down for the time being. I don't think that's the case, though.

Long term: It looks like most of Kibana will be moving to typescript, and we'll be encouraging the community to develop plugins with typescript because we'll be providing type definitions for our plugin contracts. Since we also want plugin developers to make extensive use of EUI, it makes sense for us to support typescript definitions as a first-class feature of EUI. Since contributions from the community are going to increase significantly as this is rolled out more in Kibana, it's likely we'll want to require typescript definitions for all contributions down the line.

We won't be encouraging people to use flow, and there won't be any unique benefits for doing so in our Kibana developer community, so putting the burden on the whole of EUI to maintain them seems heavy handed. The cloud codebase is an exceptional case - it's closed source and is being developed strictly by an internal group of developers, so if that specific team wants to maintain flow definitions for the components they use from EUI, they can without any fragmentation in usage of EUI in our developer community.

If there's enough demand from the developer community for flow definitions, then that's a different story. Then it's just a feature prioritization issue just like any other.

@uboness
Copy link
Contributor Author

uboness commented Mar 10, 2018

Makes sense... For me.. I just needed a reassurance that we continue moving forward with typescript in kibana. I think for the time being it makes sense that typescript users will update/maintain the type definition as they go. Once kibana moves UI to type script it'd be a good time to officially move this responsibility to the UI team.

@snide
Copy link
Contributor

snide commented Mar 29, 2018

Maybe something like this could work. cc @mattapperson who brought it up in channel.

https://www.npmjs.com/package/react-to-typescript-definitions

@uboness
Copy link
Contributor Author

uboness commented Mar 29, 2018

IMO that will do injustice to the ts definitions. With ts you can be much more expressive than with propTypes. (Eg. use generics)

@mattapperson
Copy link
Contributor

@uboness propTypes can be more expressive then generics as well. Is there something you could point to as an example of an ideal with react components having a definition?

@uboness
Copy link
Contributor Author

uboness commented Mar 30, 2018

I need to be clearer with my comments :)

  • TS is more expressive than prop types wrt to declarative statements. prop-types does provide you an escape hatch when its declarative definitions are insufficient in the form of validation functions. (A good example for when you'd use it is when you want to constraint usage of some props based on other props). That said, any tool that converts prop-types to ts, will surely only be bound to converting the declarative parts of the prop types.

  • even if prop-types would have a declarative feature that is more expressive than what TS can provide you, surely such a conversion tool cannot convert that feature to TS :)

  • For simple, low level components, you probably won't encounter many (or any) issues with auto conversion.

  • For HoC you may want to utilize TS features such as generic to provide better definitions. An example for that is EuiBasicTable (and its counterparts). This table assumes that all items you're showing are of the same type and adhere to the same schema. Lets say you have a table that lists users. While you can, in general have PropTypes.arrayOf(PropTypes.instanceOf(User)) this will only limited to the items that you pass in. A lot of other properties that can and should be aware of this type cannot be declaratively made aware of it... for example, all the callback methods that accept a user. consider the following snippet of an idea TS definition for EuiBasicTable:

declare module '@elastic/eui' {

  export type ItemCallback<Item, Return> = (item: Item) => Return;
  export type ItemValueCallback<Value, Item, Return> = (value: Value, item: Item) => Return;

  export namespace EuiBasicTable {

    export type ColumnDataType = 'string' | 'number' | 'boolean' | 'date';

    export interface FieldDataColumn<Item> {
      field: string;
      name: string;
      description?: string;
      dataType?: ColumnDataType;
      width?: string;
      sortable?: boolean;
      align?: HorizontalAlignment;
      truncateText?: boolean;
      render?: ItemValueCallback<any, Item, ReactNode>;
    }

    export interface ComputedColumn<Item> {
      render: ItemCallback<Item, ReactNode>;
      name?: string;
      description?: string;
      width?: string;
      truncateText?: boolean;
    }

    export interface DefaultItemAction<Item> {
      name: string;
      description: string;
      onClick: ItemCallback<Item, void>;
      type?: 'button' | 'icon';
      available?: ItemCallback<Item, boolean>;
      enabled?: ItemCallback<Item, boolean>;
      icon?: IconType | ItemCallback<Item, IconType>;
      color?: ButtonIconColor | ItemCallback<Item, ButtonIconColor>;
    }

    export interface CustomItemAction<Item> {
      render: (item: Item, enabled: boolean) => ReactNode;
      available: ItemCallback<Item, boolean>;
      enabled: ItemCallback<Item, boolean>;
    }

    export type ItemAction<Item> = DefaultItemAction<Item> | CustomItemAction<Item>;

    export interface ActionsColumn<Item> {
      actions: ItemAction<Item>[];
      name?: string;
      description?: string;
      width?: string;
    }

    export type Column<Item> = FieldDataColumn<Item> | ComputedColumn<Item> | ActionsColumn<Item>;

    export type ItemId<Item> = string | ((item: Item) => string);

    export interface Selection<Item> {
      itemId: ItemId<Item>;
      onSelectionChanged?: (selection: Item[]) => void;
      selectable?: ItemCallback<Item, boolean>;
      selectableMessage?: ItemValueCallback<boolean, Item, string>;
    }

    export interface Sorting {
      sort?: PropertySort;
    }

    export interface Pagination {
      pageIndex: number;
      pageSize: number;
      totalItemCount: number;
      pageSizeOptions?: number[]
    }

    export interface DataCriteria {
      page?: {
        index: number;
        size: number;
      },
      sort?: PropertySort
    }

  }

  export interface EuiBasicTableProps<Item> {
    items: Item[];
    columns: EuiBasicTable.Column<Item>[];
    pagination?: EuiBasicTable.Pagination;
    sorting?: EuiBasicTable.Sorting;
    selection?: EuiBasicTable.Selection<Item>;
    onChange?: (criteria: EuiBasicTable.DataCriteria) => void;
    error?: string;
    loading?: boolean;
    noItemsMessage?: string;
    className?: string;
  }

  export class EuiAbstractBasicTable<Item> extends React.Component<CommonProps & EuiBasicTableProps<Item>, {}> {}

  export class EuiBasicTable extends EuiAbstractBasicTable<any> {}
}

So restating my previous comment - If we can use an automated tool to convert prop-types to TS definitions, that'd be great... but with that, we'll need to make sure we have an escape hatch to override it and create our own definitions for any component we see fit.

@mattapperson
Copy link
Contributor

@uboness ah I see what your saying now. Thanks for the additional detail :)

I think I see the options as:

  1. if EUI is to continue to be written in JS not TS, then imho this becomes the eternal issue of docs staying in sync with API. Not that it cant be done when documenting manually, but I am just yet to see any API stay perfectly 100% in sync with an API in this kind of scenario. Not just definitions, but any doc really.
  2. If EUI is written in TS (the only way I personally see as a reliable way to keep EUI 100% in sync with it's API using an advanced case like you provided) EUI team has to learn TS this way... idk how people feel about that... but we could then generate typings for flow (https://github.com/joarwilk/flowgen)
  3. If we use more generic typings on JS based EUI via 100% automation then we guarantee the API is always in sync.
  4. We can go with automation + escape hatches, but those escape scenarios could easily fall out of sync (just like any docs)

In sounds like you @uboness favor option 4? Did I forget an option or did I misstate any of the above?

@uboness
Copy link
Contributor Author

uboness commented Mar 30, 2018

@mattapperson I think you're pretty much spot on.

In sounds like you @uboness favor option 4? Did I forget an option or did I misstate any of the above?

In a way... I do hoping that most of the low level components will be auto-synced and for the HoC we'll be responsible enough to update it occasionally (I don't think it's the end of the world if the type defs are a bit behind, as long as when ppl pick up on missing defs/props, they'll go and add them... yes, it requires discipline)

If EUI is written in TS (the only way I personally see as a reliable way to keep EUI 100% in sync with it's API using an advanced case like you provided) EUI team has to learn TS this way... idk how people feel about that... but we could then generate typings for flow

we've discussed this in the past, and I think the general direction is that eventually we would want to move to TS across the board. But that will take time, and at this point, there's so much (more important) work to be done in EUI that it's really not a high priority now. That said, I would agree that it'll be great if the EUI engs will start looking and learning TS and maybe even try to use it and build a few components in TS... they might like what they'll see and before you know it, TS will be the new default.

@snide snide mentioned this issue Apr 30, 2018
Closed
44 tasks
@chandlerprall chandlerprall mentioned this issue Oct 24, 2018
Closed
6 tasks
@chandlerprall
Copy link
Contributor

Closing this. The component list&progress is out of date, and EUI now supports authoring components in TypeScript - we are starting to convert components over.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@uboness uboness

@weltenwort weltenwort

Labels
meta typescript
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@epixa @mattapperson @uboness @chandlerprall @snide @weltenwort @pugnascotia