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.

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

[GSoC] Automatic Documentation System for TypeScript Components #3056

Closed
thompsongl opened this issue Mar 12, 2020 · 50 comments · Fixed by #3554
Closed

[GSoC] Automatic Documentation System for TypeScript Components #3056

thompsongl opened this issue Mar 12, 2020 · 50 comments · Fixed by #3554
Assignees

Comments

@thompsongl
Copy link
Contributor

thompsongl commented Mar 12, 2020

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


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

@elastic elastic locked and limited conversation to collaborators Mar 12, 2020
@elastic elastic unlocked this conversation May 11, 2020
@thompsongl
Copy link
Contributor Author

@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.

@ashikmeerankutty
Copy link
Contributor

@thompsongl Thanks!

@ashikmeerankutty
Copy link
Contributor

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

@thompsongl
Copy link
Contributor Author

What do you think, @chandlerprall?

@chandlerprall
Copy link
Contributor

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

@ashikmeerankutty
Copy link
Contributor

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

@chandlerprall
Copy link
Contributor

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.

@ashikmeerankutty
Copy link
Contributor

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.

@ashikmeerankutty
Copy link
Contributor

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

image

@chandlerprall
Copy link
Contributor

chandlerprall commented May 27, 2020

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

} & CommonProps &
ExclusiveUnion<WithIconOnClick, {}> &
ExclusiveUnion<
ExclusiveUnion<WithButtonProps, WithAnchorProps>,
WithSpanProps
>;

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.

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented May 27, 2020

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

@ashikmeerankutty
Copy link
Contributor

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented May 28, 2020

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"
    }
  }
}

@ashikmeerankutty
Copy link
Contributor

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

@chandlerprall
Copy link
Contributor

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

@thompsongl
Copy link
Contributor Author

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?

@chandlerprall
Copy link
Contributor

the webpack loader

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

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented Jun 3, 2020

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).

@ashikmeerankutty
Copy link
Contributor

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. 😅

@ashikmeerankutty
Copy link
Contributor

image

I think time taken is bit long 😅

@chandlerprall
Copy link
Contributor

43 minutes is just a little much 😁

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented Jun 4, 2020

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?

@ashikmeerankutty
Copy link
Contributor

Also, there are some warnings

image

@chandlerprall
Copy link
Contributor

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.

@chandlerprall
Copy link
Contributor

chandlerprall commented Jun 9, 2020

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

@ashikmeerankutty
Copy link
Contributor

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

@chandlerprall
Copy link
Contributor

Let's start with,

ReactNode to render as this component's children

@ashikmeerankutty
Copy link
Contributor

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

@chandlerprall
Copy link
Contributor

chandlerprall commented Jun 12, 2020

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

@ashikmeerankutty
Copy link
Contributor

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

Yes

@ashikmeerankutty
Copy link
Contributor

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.

@ashikmeerankutty
Copy link
Contributor

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

@ashikmeerankutty
Copy link
Contributor

It seems like this change

props: {
EuiScreenReaderOnly: ScreenReaderOnlyDocsComponent,
},

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

@ashikmeerankutty
Copy link
Contributor

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

@thompsongl
Copy link
Contributor Author

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

What was the reason for adding the ScreenReaderOnlyDocsComponent?

@chandlerprall
Copy link
Contributor

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

Yes

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

@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented Jun 15, 2020

What was the reason for adding the ScreenReaderOnlyDocsComponent?

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

@thompsongl
Copy link
Contributor Author

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

@thompsongl
Copy link
Contributor Author

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.

@ashikmeerankutty
Copy link
Contributor

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 to guideSection 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.

@thompsongl
Copy link
Contributor Author

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

@thompsongl
Copy link
Contributor Author

Reopening this so all the tracking continues in one issue.

@ashikmeerankutty, please list any follow-up work to #3554 that you plan on doing (#3554 (review); sorting the props, for instance)

@thompsongl thompsongl reopened this Aug 13, 2020
@ashikmeerankutty
Copy link
Contributor

ashikmeerankutty commented Aug 13, 2020

Follow ups

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

@cchaos
Copy link
Contributor

cchaos commented Aug 25, 2020

@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!

@thompsongl
Copy link
Contributor Author

@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 🚀

@thompsongl
Copy link
Contributor Author

🙌

@ashikmeerankutty
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants