docgen: Add TypeScript support #29189
Conversation
|
👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @sarayourfriend! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other. If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information. |
|
Size Change: 0 B Total Size: 1.4 MB ℹ️ View Unchanged
|
| return getReturnTypeAnnotation( tag, token ); | ||
| } | ||
| default: { | ||
| return tag.type; |
There was a problem hiding this comment.
I lifted the tag.type check here and that uncovered this path: this is when a type wasn't documented by JSDoc and there's no TS info we can extract from the ASTNode either. I suppose return an empty string or "unknown type" should be fine here.
There was a problem hiding this comment.
Perhaps empty is a better choice to follow what's done at https://github.com/WordPress/gutenberg/pull/29189/files?file-filters%5B%5D=.js&file-filters%5B%5D=.md&file-filters%5B%5D=.tsx&hide-deleted-files=true#diff-5c6936b9e1276b628a49212453264bc8f663f1f964dea9badeefb73216f4a8c5R338
There was a problem hiding this comment.
That's a good point. I think in that case an empty string is fine.
| * @param {babelTypes.TSImportType} typeAnnotation | ||
| */ | ||
| function getImportTypeAnnotation( typeAnnotation ) { | ||
| // Should this just return the unqualified name (i.e., typeAnnotation.name || typeAnnotation.right.name)? |
There was a problem hiding this comment.
This is still an outstanding question to me... I think we should just use the unqualified name without the import, for stylistic reasons, but maybe using the import would be best. In any case, import types are rare in actual TypeScript so it wouldn't happen often. The key is to use descriptive and contextually appropriate type names anyway.
|
|
||
| /** | ||
| * @param {TypeAnnotation} typeAnnotation | ||
| * @return {string | null} The type or null if not an identifiable type. |
There was a problem hiding this comment.
| * @return {string | null} The type or null if not an identifiable type. | |
| * @return {string} The type or an empty string if not an identifiable type. |
| case 'TSBooleanKeyword': { | ||
| return 'boolean'; | ||
| } | ||
| case 'TSConditionalType': { |
There was a problem hiding this comment.
Couldn't figure out how to make one of these. If anyone knows, would be much appreciated 😃
| case 'TSConstructorType': { | ||
| return `new ${ getFunctionTypeAnnotation( typeAnnotation ) }`; | ||
| } | ||
| case 'TSExpressionWithTypeArguments': { |
There was a problem hiding this comment.
Same here, no idea what this expression with type arguments is 🤔
| typeAnnotation.parameterName.name | ||
| } is ${ getTypeAnnotation( typeAnnotation.typeAnnotation ) }`; | ||
| } | ||
| case 'TSTypeQuery': { |
There was a problem hiding this comment.
Don't know what this is either
| return getReturnTypeAnnotation( tag, token ); | ||
| } | ||
| default: { | ||
| return tag.type; |
There was a problem hiding this comment.
That's a good point. I think in that case an empty string is fine.
|
I think this is a good compromise: it addresses the concerns about having different parses for code and documentation and only requires us to add new TS types when we update the babel version if there are new ones. 👏 I was reading the code and pushed a few things to understand and clarify the flow, I hope you don't mind. The only blocker I see for this to land is having tests. Happy to help with this. To land this sooner, we don't need to cover all the TS types available via babel, though, we can just add in this PR the flow + the TS types in use by the |
|
https://astexplorer.net is the best place I've found, just make sure to choose the |
|
@nosolosw I took a shot at adding a whole slew of tests. Let me know what you think 😀 |
|
@nosolosw I realized yesterday that this doesn't support variable declaration exports. For example: const pure: HigherOrderComponent = createHigherOrderComponent(...);
export default pure;Becuase the It's hard though because we don't have any examples of this currently to try out. Did NOT mean to close the PR, whoops. |
e313eda to
e9bc2b3
Compare
|
I was about to review this and saw it required a rebase from |
e9bc2b3 to
04904a2
Compare
|
In terms of docgen, changes this looks fine. Although we don't have all type possibilities documented or tested is fine as there's a structure to add them later, which I think we have now. Thank you especially for the I think this is ready to land, although I'd like us to take a look at the following before merging:
|
|
Thanks for taking a look at this @nosolosw. I'll address your feedback and set this up for merging soon! |
04904a2 to
5b66b71
Compare
Didn't break anything but we did accidentally duplicate the
This is because the return type was a TypeReference where the typeName property was a QualifiedName. This wasn't properly supported (an oversight on my part) but it is fixed now as of 5b66b71 |
|
The CHANGELOG and README have been updated. We now have two major releases stacked for |
Description
Adds TS support to docgen. This does not require adding any new parsers, the currently used parsers are sufficient (🎉).
All of the new logic exists inside of
get-type-annotation. This basically consists of a huge switch function over all of the possible types of type annotation nodes. The module then opinionatedly formats the type annotation based on the AST and returns the new type annotation. If a type annotation already exists (for example, in the case of JSDoc) it just uses that instead.This does require explicit typing of all exported function returns, however, which is why I added the return type annotation to
react-i18n.docgenisn't running any actual TypeScript engine so there's no way to infer the return type like TS normally would be able to do. However, I don't see this as a shortcoming but as a feature: it forces us to explicitly document the inputs and outputs of our exported functions (ESLint should enforce this anyway, once #27143 is merged.Most of this code is currently unused. Testing it is going to be a chore, so if someone wants to jump in and help me with that, I'd love the assistance (but of course is not necessary). I'll slowly add some unit tests over the next week or so. If anyone has suggestions for how/what exactly is worth testing in this new area, please let me know.
Note: I did remove a nonsense param annotation in the first commit. Like I said, it was a nonsense annotation so I think it's safe to remove it.
How has this been tested?
Run
npm run docs:build. Ensure that there are no changes except toreact-i18n.Types of changes
Adds support for TypeScript type annotations without affecting existing JSDoc support.
Checklist: