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

Addon-docs: Support jsdoc params to describe function signature #8660

Merged

Conversation

patricklafrance
Copy link
Member

@patricklafrance patricklafrance commented Oct 31, 2019

Issue: #8532

What I did

This PR add JSDoc support to describe the signature of a function in the docs addon props table when using PropTypes.

When a prop type is defined as a function, if the optional JSDoc is provided, the type of the prop will be a function signature infered from the JSDoc @param and @returns tags instead of being hardcoded to "func".

The support for @param and @returns follow the JSDoc specs:

https://jsdoc.app/tags-param.html
https://jsdoc.app/tags-returns.html

This PR is a base for other issues and enhancement that will be tackled soon.

There is a few considerations that you should be aware of...

Limitations:

A property description cannot start with a @.

This means that for the property:

  /**
   * @onClick description
   */
  onClick: PropTypes.func

The description would be empty.

When @ is in the body of the description it's fine.

  /**
   * onClick @description@
   */
  onClick: PropTypes.func

Would display: onClick @description@

JSDoc parser:

To parse the JSDoc, I found 2 popular packages: https://www.npmjs.com/package/doctrine and https://www.npmjs.com/package/comment-parser

doctrine is obviously the most popular one with 14 millions weekly downloads. Sadly, it has been deprecated a few months ago.

The ESLint team decided to stop supporting rules to validate JSDoc format.

comment-parser is an interesting option but it doesn't work in a web environment, it has a dependency on node fs.

I decided to use doctrine even if it's deprecated since it's still widely use and is stable. I don't think we will end up supporting something else than @param, @returns and @ignore which works fine with doctrine. I dont see the JSDoc specs change for those tag in a near future.

Parsing the JSDoc:

To ensure good perfomance, the JSDoc tags should probably be parsed at compile time with a babel plugin. To get something out quickly I parse them from the Props React component.

Performance seems acceptable for now. 3 tables of 50 props having JSDoc tags render instantly in dev. A test is available in the cra-ts-kitchen-sink.

What's next?

How to test

  • Jest tests are avaiable: yarn jest --testPathPattern=type-system-handlers --watch
  • Performance tests are available in cra-ts-kitchen-sink under the story "docgen/Props types/jsdoc-perfo"
  • 43 rendering cases are available cra-ts-kitchen-sink under the story "docgen/Props types/jsdoc"

@vercel
Copy link

vercel bot commented Oct 31, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/95435c2m3
🌍 Preview: https://monorepo-git-8532-support-jsdocs-params-to-describ-caebd7.storybook.now.sh

@patricklafrance
Copy link
Member Author

I am not sure how I ended up with all those changes to the cra-ts-kichen-sink @shilman .

I only changed the stories.

@shilman
Copy link
Member

shilman commented Nov 1, 2019

@patricklafrance This is amazing work. 🥇

The changes are there because you started on my branch, which already upgraded cra-ts-kitchen-sink prior to your changes.

I'll review it all properly today or this weekend, but at first glance it looks superb!!!

@shilman shilman changed the title 8532 support jsdocs params to describe js function signature Addon-docs: Support jsdocs params to describe function signature Nov 1, 2019
@shilman shilman changed the title Addon-docs: Support jsdocs params to describe function signature Addon-docs: Support jsdoc params to describe function signature Nov 1, 2019
@@ -59,6 +60,9 @@
"ts-dedent": "^1.1.0"
},
"devDependencies": {
"@types/doctrine": "^0.0.3",
"@types/enzyme": "^3.10.3",
Copy link
Member

Choose a reason for hiding this comment

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

Is this needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Without those types I couldn't compile the app with yarn bootstrap

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

This is tremendous. Totally blown away!!! ❤️

Minor comments only...

? propsFromDocgen(processedType, section)
: propsFromPropTypes(processedType, section);

return extractPropsFromDocgen(processedType, section);
Copy link
Member

Choose a reason for hiding this comment

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

Is propsFromPropTypes unused? Is this a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, I see it below. Is this tested?

Copy link
Member

Choose a reason for hiding this comment

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

Nvm, I see it is! 😻

[TypeSystem.Unknown]: unknownHandler,
};

export const getPropTypeSystem = (docgenInfo: DocgenInfo): string => {
Copy link
Member

@shilman shilman Nov 1, 2019

Choose a reason for hiding this comment

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

This is wonderful 🙆‍♂

@vercel vercel bot requested a deployment to staging November 1, 2019 15:03 Abandoned
@shilman shilman merged commit 4aed2fe into next Nov 1, 2019
@shilman shilman deleted the 8532-support-jsdocs-params-to-describe-js-function-signature branch November 1, 2019 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants