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

TypeScript support #348

Merged
merged 12 commits into from
May 3, 2019
Merged

TypeScript support #348

merged 12 commits into from
May 3, 2019

Conversation

devongovett
Copy link
Contributor

@devongovett devongovett commented Apr 22, 2019

Closes #325.
Depends on benjamn/ast-types#330

This adds TypeScript support to react-docgen. It automatically replaces the flow plugin with the typescript plugin in babel-parser when a .ts or .tsx file is seen.

The output follows the existing flow output format since TypeScript types are quite similar to Flow. The implementation follows the existing flow implementation quite closely as well - a new getTSType file is added similar to the getFlowType one, and the surrounding code is updated to also handle TypeScript annotations in addition to Flow.

This brings up several questions about API and code organization.

  1. Do we want to change the output format key from flowType to something more generic, add an additional tsType key, or just leave it as is?
  2. Internally, do we want to leave the files/function names that now handle both flow and typescript with flow in the name, or change them to be more generic as well?

A few things left to do on this:

  • Support interfaces in addition to type aliases. This is also not currently the case for Flow, however...
  • Ensure that all TypeScript types are handled
  • Handle method documentation for TypeScript
  • Decide on questions above

@danez
Copy link
Collaborator

danez commented Apr 23, 2019

Thank you for working on this. This is amazing.

  1. Do we want to change the output format key from flowType to something more generic, add an additional tsType key, or just leave it as is?

This is a tricky problem. I'm not sure about a clear answer. If we would add the docs for both type systems to one key we need to make sure they are always compatible and always stay compatible. Additional features which do not exist in the other language are okay I guess, but features that just would need a slightly different documentation could make things complicated. Maybe for example differences in enum types or mapped types? I can't think of any concrete example but I wonder if it would make it complicated for tools using react-docgen which support components written in flow and ts.
Not sure if this is a real problem maybe you know more if there are any documentation differences in types between flow and ts?
If we would use different keys for ts and flow we wouldn't have this problem at all.

  1. Internally, do we want to leave the files/function names that now handle both flow and typescript with flow in the name, or change them to be more generic as well?

I think we should rename it, but we don't have to do it in this PR. I would also classify this a breaking change, because I saw some custom resolvers using internal utils of react-docgen.

Also in addition what I said to 1. what do you think about adding a key to the documentation that indicates which language we were parsing, js, flow or typescript? But also not sure if this would be useful to users.

Maybe @okonet or @sapegin have input about what would make it easy to use in react-styleguidist?

@devongovett
Copy link
Contributor Author

Thanks for your thoughts on this. I think we should have a separate tsType (or typescript?) key from flowType for future extensions, though the format will probably be the same as flow initially. This way it is also not a breaking change.

@thebuilder
Copy link

I think it also makes the most sense to keep flowType as is, and introduce tsType.
If you are consuming the output, then you already have the handle both PropTypes and flowType. Adding an extra key, that hopefully matches the structure of flowType, should be easy enough to deal with.

@devongovett
Copy link
Contributor Author

devongovett commented Apr 28, 2019

I think this is ready for review! I adjusted the output to use tsType for TypeScript and retainflowType for Flow. They use the same output format at the moment though. I didn't rename any of the internal files or methods for flow that now also handle TypeScript since @danez mentioned that would be a breaking change.

Also added support for generics to both Flow and TypeScript. It will inline type parameters when merging extended interfaces or other type references. This should help allow you to reuse generic types across components while keeping the docs understandable.

Only waiting on recast to update their dep on ast-types: benjamn/recast#590

@devongovett devongovett changed the title [WIP] TypeScript support TypeScript support Apr 28, 2019
@devongovett devongovett marked this pull request as ready for review April 28, 2019 03:53
Copy link
Collaborator

@danez danez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. It took some time to look through everything. I added some inline comments about some small changes and one question about the documentation of function signatures, but besides that it looks nice and clean to me.

declaration: NodePath,
instantiation: NodePath,
inputParams: ?TypeParameters,
) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a return type here? Which I guess is TypeParameters?

? paramPath.get('default')
: null;
const typePath =
i < instantiation.node.params.length
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

micro optimizing: The whole length lookup could be done before the loop.

src/utils/getTSType.js Show resolved Hide resolved
@@ -43,7 +43,7 @@
"commander": "^2.19.0",
"doctrine": "^3.0.0",
Copy link
Collaborator

@danez danez May 2, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update @babel/core to ^7.4.4 so we ensure that typescript is fully supported.

Also noticed we have it in dependencies and dev dependencies, so I guess we can remove it from dev.

@danez danez added this to the v5 milestone May 2, 2019
@devongovett
Copy link
Contributor Author

Thanks for the review @danez! Updated based on your comments.

@devongovett
Copy link
Contributor Author

So I upgraded babel-core, and am now getting a bunch of errors about undefined exports in the tests... :|

@danez
Copy link
Collaborator

danez commented May 3, 2019

I will have a look

@danez danez merged commit d1b07c4 into reactjs:master May 3, 2019
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript support
5 participants