Eui: [GSoC] Automatic Documentation System for TypeScript Components

Created on 12 Mar 2020  路  50Comments  路  Source: elastic/eui

A better automatic system for documenting EUI TypeScript components. Specifically, we're looking for a better method to extract the acceptable values for each prop on the API. This would be done through automatic transversal of the EUI components itself, combined with the type values from TS and our own inline comments.

Outcome
More accurate, robust prop documentation within the existing "Props" tab for each example section in component documentation. As the EUI team is nearing completion of complete TypeScript conversion, it is possible that the current type and prop traversal system can be entirely replaced.

Related Discussion

  • #1688; sometimes manual documentation will still be needed/helpful

_Interested in working on EUI for Google Summer of Code? See more details here: https://github.com/elastic/gsoc_

GSoC

Most helpful comment

@cchaos @thompsongl @chandlerprall Thanks for all your support. It was wonderful working with you 馃槂

All 50 comments

@ashikmeerankutty We'll use this issue to discuss and track modifications to your proposal.

Comment in this thread and then I can assign the issue to you.

@thompsongl Thanks!

Should I continue this as a separate project outside eui and use the plugin in eui or shift the plugin development inside eui.

What do you think, @chandlerprall?

Let's move it inside eui; makes it easier to observer changes against any set of components

Out of curiosity! I just tried adding the plugin to eui. Few observations:

  • It's taking a lot of time.
  • Docs info is generated with most of the components.
  • Build is broken

It's taking a lot of time.

Sounds like it's processing TypeScript code 馃榾

Docs info is generated with most of the components.

Sweet!

Build is broken

The only way to make things better :)

Did you remove the existing ./scripts/babel/proptypes-from-ts-props plugin from babel's config? If not, it might be causing interference.

Next week, I can help look into any issues you're not sure about. If this is too much of a problem right now, there's no rush on moving it into EUI.

Did you remove the existing ./scripts/babel/proptypes-from-ts-props plugin from babel's config? If not, it might be causing interference.

No, I will try removing that.

Next week, I can help look into any issues you're not sure about. If this is too much of a problem right now, there's no rush on moving it into EUI.

It doesn't seem to be trouble. I think it's better to continue in eui. I will get a lot of components to try.

Just found a webpack loader that uses react-docgen-typescript 馃槓.
Tried the same in eui and the results seems good 馃槓.

image

How well does that loader / react-docgen-typescript work with components like EuiButton, which has a mix of our custom PropsForAnchor, PropsForButton, and ExclusiveUnion types? Or EuiBadge which nests those concepts https://github.com/elastic/eui/blob/c9ae5a08eab5729af892d00abda9365ff96d2a44/src/components/badge/badge.tsx#L97-L102

Second aspect of react-docgen-typescript that wasn't great in the past was whether or not it supports types from 3rd party libs / node_modules. Ideally, I think we'd want to white list some like react-beautiful-dnd, either including the types directly or even maybe creating a link to their documentation.

And we also don't want to lose support for generating prop types.

With EuiButton and EuiBadge all props are found except the onClick prop 馃槄. The reason onClick was not found is due to filtering props from node-modules.

react-docgen-typescript support types from node-modules. I tried it with EuiDraggable it displays props from react-beautiful-dnd

image

react-docgen-typescript allows a propsFilter option.
We could use this to whitelist certain props by name or whitelist props from certain files.

Example of a prop

{
  "prop": {
    "defaultValue": null,
    "description": "",
    "name": "className",
    "parent": {
      "fileName": "eui/node_modules/react-ace/lib/ace.d.ts",
      "name": "IAceEditorProps"
    },
    "required": false,
    "type": {
      "name": "string | undefined"
    }
  }
}

@chandlerprall Should I proceed in this way or with the plugin.

@thompsongl what do you think, especially in context of feeding these results/data into the docs stuff?

react-docgen-typescript allows a propsFilter option.

Is this the same for the webpack loader? What deficiencies (if any) does the webpack loader have when compared with the plugin?

the webpack loader

Good catch, I dropped that context. We'd (ideally) need a way to get those results without running webpack

Is this the same for the webpack loader? What deficiencies (if any) does the webpack loader have when compared with the plugin?

Yes, the webpack-loader provides all the functionalities of react-docgen-typescript. I haven't found any deficiencies with the loader. But I hope creating a custom plugin gives more control on the parser especially for some eui specific needs (if any).

Good catch, I dropped that context. We'd (ideally) need a way to get those results without running webpack

Thanks for the clarification. I should continue with the plugin. 馃槄

image

I think time taken is bit long 馃槄

43 minutes is just a _little_ much 馃榿

With some optimization, I was able to decrease it to 2 minutes 馃榿 . But there are issues with some components

image

Should I start a PR now?

Also, there are some warnings

image

Should I start a PR now?

Sure!

Also, there are some warnings

Those are coming from the typescript types/interfaces being imported between files, but those values don't exist after the typescript preset takes its pass. We added logic in our custom proptypes-from-ts-props babel plugin. Feel free to ignore these for now, I'm not sure how we want to handle it now vs. in the future.

From my observation, CommonProps is used for generating prop information. With the new system, we could whitelist some of the props. So should I remove CommonProps from components? and whitelist those props in CommonPropsinside the plugin itself

Couple thoughts:

  • yes, would be great if the plugin is configured to auto-include these when available, even when CommonProps is not part of a component - e.g. only HTMLAttributes<HTMLElement>
  • data-test-subj only exists on CommonProps, any auto-enabling of it would need to somehow verify it is properly handled (maybe we can extend React's HTMLAttributes to have data-test-subj?)

react-docgen-typescript only find children prop with a comment. So should I add comments to children prop ?

Yes, we'll want to make sure any existing children entries in _Props_ remain; if that requires adding comments let's do so

@chandlerprall I can`t think of a suitable comment for the same. Can you please suggest one 馃槓

Let's start with,

ReactNode to render as this component's children

There is also an issue with react-docgen-typescript It doesn't produce a result when class extended as React.Component<>.

Does it produce results with simple Component<> (imported from React)?

Does it produce results with simple Component<> (imported from React)?

Yes

I'm facing a weird issue in my plugin. I couldn't figure it till now. Exported types from tsx components aren't working whereas exported types from ts files work fine. It's not an issue related to react-docgen-typescript as I tried the same with my first babel plugin ( that takes 45mins 馃槓 ). It is working well with tsx files. As a quick workaround, I could move the types to be exported to a new file like types.ts so that it will work well. But I think it's worth fixing this issue inside the plugin itself.
The issue can be found in component such as EuiSuggest.

The above issue was fixed using jsx: ts.JsxEmit.React in compiler option for createProgram 馃槂

It seems like this change
https://github.com/elastic/eui/blob/1259952aa509d8e7c55e915e2f3e8932f475193e/src-docs/src/views/accessibility/accessibility_example.js#L119-L121
is not necessary while using the new plugin. Can I change it ?

Manually checked all components to make sure that props are generated. Props are generated for 90% components even though there are issues with some prop types.
Props are not generated for components with forwardRef and that was fixed in a latest PR in react-docgen-typescript

is not necessary while using the new plugin. Can I change it ?

What was the reason for adding the ScreenReaderOnlyDocsComponent?

Does it produce results with simple Component<> (imported from React)?

Yes

We can convert all React.Component<> to Component<>, that's fine!

What was the reason for adding the ScreenReaderOnlyDocsComponent?

The EuiScreenReaderOnly component uses cloneElement and that was creating issues with react-docgen I guess

The EuiScreenReaderOnly component uses cloneElement and that was creating issues with react-docgen I guess

Go ahead and change it if it is no longer the source of problems

This would be follow up work to the PR you already have open, but we'd also like to find a way to directly include types and interfaces in the Props table (see #1688)

Tables are good example of this. We manually add complex types and interfaces through a propsInfo hack so that consumers can see subcomponent types in the table. For instance, Pagination is a valuable type, but one that wouldn't come through via simply including EuiTableProps.

One workaround for this issue I just found was to create a component called TableProps or OtherProps and let its component extend interfaces that we could display like for the table example:

import React, { FunctionComponent } from 'react';
import { Pagination } from './pagination_bar';
import { EuiTableSortingType } from './table_types';

export interface OtherProps extends Pagination, EuiTableSortingType<any> {}

export const TableProps: FunctionComponent<OtherProps> = () => <div />;

In the generated docgenInfo we could find extendedInterfaces that have all the extended props of the component as separate info.

We could then pass it like TableProps.extendedInterfaces.Pagination toguideSection Page

```
{
"description": "",
"displayName": "TableProps",
"methods": [],
"props": {
"pageIndex": {
"defaultValue": null,
"description": "",
"name": "pageIndex",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": true,
"type": {
"name": "number"
}
},
"pageSize": {
"defaultValue": null,
"description": "",
"name": "pageSize",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": true,
"type": {
"name": "number"
}
},
"totalItemCount": {
"defaultValue": null,
"description": "",
"name": "totalItemCount",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": true,
"type": {
"name": "number"
}
},
"pageSizeOptions": {
"defaultValue": null,
"description": "",
"name": "pageSizeOptions",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": false,
"type": {
"name": "number[]"
}
},
"hidePerPageOptions": {
"defaultValue": null,
"description": "",
"name": "hidePerPageOptions",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": false,
"type": {
"name": "boolean"
}
},
"sort": {
"defaultValue": null,
"description": "",
"name": "sort",
"parent": {
"fileName": "eui/src/components/basic_table/table_types.ts",
"name": "EuiTableSortingType"
},
"required": false,
"type": {
"name": "{ field: string | number | symbol; direction: \"asc\" | \"desc\"; }"
}
},
"allowNeutralSort": {
"defaultValue": null,
"description": "",
"name": "allowNeutralSort",
"parent": {
"fileName": "eui/src/components/basic_table/table_types.ts",
"name": "EuiTableSortingType"
},
"required": false,
"type": {
"name": "boolean"
}
},
"enableAllColumns": {
"defaultValue": null,
"description": "",
"name": "enableAllColumns",
"parent": {
"fileName": "eui/src/components/basic_table/table_types.ts",
"name": "EuiTableSortingType"
},
"required": false,
"type": {
"name": "boolean"
}
}
},
"extends": [],
"extendedInterfaces": {
"Pagination": {
"__docgenInfo": {
"description": "",
"displayName": "Pagination",
"props": {
"pageIndex": {
"defaultValue": null,
"description": "",
"name": "pageIndex",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": true,
"type": {
"name": "number"
}
},
"pageSize": {
"defaultValue": null,
"description": "",
"name": "pageSize",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": true,
"type": {
"name": "number"
}
},
"totalItemCount": {
"defaultValue": null,
"description": "",
"name": "totalItemCount",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": true,
"type": {
"name": "number"
}
},
"pageSizeOptions": {
"defaultValue": null,
"description": "",
"name": "pageSizeOptions",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": false,
"type": {
"name": "number[]"
}
},
"hidePerPageOptions": {
"defaultValue": null,
"description": "",
"name": "hidePerPageOptions",
"parent": {
"fileName": "eui/src/components/basic_table/pagination_bar.tsx",
"name": "Pagination"
},
"required": false,
"type": {
"name": "boolean"
}
}
}
}
},
"EuiTableSortingType": {
"__docgenInfo": {
"description": "",
"displayName": "EuiTableSortingType",
"props": {
"sort": {
"defaultValue": null,
"description": "",
"name": "sort",
"parent": {
"fileName": "eui/src/components/basic_table/table_types.ts",
"name": "EuiTableSortingType"
},
"required": false,
"type": {
"name": "{ field: string | number | symbol; direction: \"asc\" | \"desc\"; }"
}
},
"allowNeutralSort": {
"defaultValue": null,
"description": "",
"name": "allowNeutralSort",
"parent": {
"fileName": "eui/src/components/basic_table/table_types.ts",
"name": "EuiTableSortingType"
},
"required": false,
"type": {
"name": "boolean"
}
},
"enableAllColumns": {
"defaultValue": null,
"description": "",
"name": "enableAllColumns",
"parent": {
"fileName": "eui/src/components/basic_table/table_types.ts",
"name": "EuiTableSortingType"
},
"required": false,
"type": {
"name": "boolean"
}
}
}
}
}
}
}
````

The issue here if two of the interface contains the same props only one among them will be shown.

Thanks for looking into it. Let's move discussion to that issue (#1688).

Reopening this so all the tracking continues in one issue.

@ashikmeerankutty, please list any follow-up work to #3554 that you plan on doing (https://github.com/elastic/eui/pull/3554#pullrequestreview-453411137; sorting the props, for instance)

Follow ups

  • [x] Sort the props table in increasing order of prop name
  • [x] Vertical scroll for functions
  • [x] Ref as a prop

@ashikmeerankutty Thank you so much for all the work you did on this project!!! 馃帀 馃帀 I know our all our Elastic engineers are extremely grateful for a more robust and reliable prop table with real Typescript types. The "Extends" bit, while seemingly small, is a game changer! Best of luck to you in future endeavors!

@ashikmeerankutty I merged #3944, which checks the last box here 馃帀

Just make sure you submit all your GSoC evaluations, etc. before the deadline later this week (if you haven't already).

@cchaos is right: we're all thrilled with how it turned out! You did fantastic work over the last few months 馃殌

馃檶

@cchaos @thompsongl @chandlerprall Thanks for all your support. It was wonderful working with you 馃槂

Was this page helpful?
0 / 5 - 0 ratings