Components: Set up README auto-generator #66035
Conversation
mirka
added
[Type] Developer Documentation
|
Size Change: 0 B Total Size: 1.77 MB ℹ️ View Unchanged
|
|
Flaky tests detected in bbdf0fb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11283259616
|
| /** | ||
| * External dependencies | ||
| */ | ||
| const docgen = require( 'react-docgen-typescript' ); |
There was a problem hiding this comment.
We're using the same docgen as the one we use in Storybook, to keep the props data as consistent as possible between Storybook and README.
| ] | ||
| : []; | ||
|
|
||
| return json2md( |
There was a problem hiding this comment.
json2md is a JS-to-Markdown conversion library.
| prop.description, | ||
| { | ||
| ul: [ | ||
| `Type: \`${ renderPropType( prop.type ) }\``, |
There was a problem hiding this comment.
For displaying the prop type, I opted to use the - Type: format rather than the fooType### propName: format, because it is more adaptable in cases where the type is long (like an enum).fooType
There was a problem hiding this comment.
Sounds sensible to me, especially as the first iteration.
We could later consider more advanced layouts (like maybe a table layout inspired from Storybook), but that's a story for another day.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| */ | ||
| const { generateMarkdownDocs } = require( './markdown' ); | ||
|
|
||
| // For consistency, options should generally match the options used in Storybook. |
There was a problem hiding this comment.
Is there a way to read those same options from the storybook config, instead of duplicating them?
There was a problem hiding this comment.
Unfortunately they're internal. I just pulled them from the source code.
| prop.description, | ||
| { | ||
| ul: [ | ||
| `Type: \`${ renderPropType( prop.type ) }\``, |
There was a problem hiding this comment.
Sounds sensible to me, especially as the first iteration.
We could later consider more advanced layouts (like maybe a table layout inspired from Storybook), but that's a story for another day.
| '<!-- This file is generated automatically and cannot be edited directly. -->\n', | ||
| { h1: typeDocs.displayName }, | ||
| { | ||
| p: `<p class="callout callout-info">See the <a href="https://wordpress.github.io/gutenberg/?path=/docs/components-${ typeDocs.displayName.toLowerCase() }--docs">WordPress Storybook</a> for more detailed, interactive documentation.</p>`, |
There was a problem hiding this comment.
Should we have similar callouts for private, deprecated, and experimental components?
There was a problem hiding this comment.
I will add one for experimental when we encounter it. But probably not for private, since private component readmes shouldn't be public on Block Editor Handbook.
There was a problem hiding this comment.
Makes sense. And what about deprecated? Same strategy as experimental?
There was a problem hiding this comment.
Right. Probably a customizable callout, for example:
gutenberg/packages/components/src/radio-group/README.md
Lines 3 to 5 in ca9857e
There was a problem hiding this comment.
Makes sense. Do you think it would make sense to include an example of an experimental component and of a deprecated component in this PR or as a quick follow-up once this PR is merged?
There was a problem hiding this comment.
Yes! After merging this first iteration I'm going to continue adding at least around ten more components so I can test and add more features as necessary. Once it feels somewhat stable we can open it up to other contributors.
| @@ -54,6 +54,8 @@ export type AlignmentMatrixControlIconProps = Pick< | |||
| * component instead_ | |||
| * | |||
| * The size of the icon. | |||
| * | |||
| * @deprecated | |||
There was a problem hiding this comment.
What happens in the generated README if a prop is @deprecated but not @ignored ? Should we somehow add that information ?
There was a problem hiding this comment.
I can't recall if we have any of these anymore. We can see when we encounter one. Maybe the answer will be to @ignore it.
| - Type: `string` | ||
| - Required: No | ||
| - Default: `Alignment Matrix Control` | ||
| - Type: `(newValue: AlignmentMatrixControlValue) => void` |
There was a problem hiding this comment.
Is there a way we can be more consistent with type expansion? In this case, for example, AlignmentMatrixControlValue is expanded for the value prop but not for the onChange prop 🤔
There was a problem hiding this comment.
It's consistent in the sense that the output pretty much matches Storybook.
But no, the docgen doesn't give me details for AlignmentMatrixControlValue in the onChange prop data, so we can't really do anything there.
There was a problem hiding this comment.
Got it. So, nothing to do for the time being. If we ever want to tweak/improve this aspect, we'll have to look at a different types extractor for both Storybook and this docgen
There was a problem hiding this comment.
Great work @mirka, excited to see this work 🚀
I've left some comments and suggestions, mostly to check for edge cases or unexpected situations, let me know what you think.
I also wonder if in the long-term we're aiming to automate the generation of the doc manifest of each component. Seems like ideally we should be able to extract this information automatically, no?
| @@ -0,0 +1,53 @@ | |||
| function renderPropType( type ) { | |||
| switch ( type.name ) { | |||
| case 'enum': { | |||
There was a problem hiding this comment.
Maybe. I'll add more logic as I encounter them.
| function renderPropType( type ) { | ||
| switch ( type.name ) { | ||
| case 'enum': { | ||
| const MAX_ENUM_VALUES = 10; |
There was a problem hiding this comment.
Should this const be declared up there at the module level?
There was a problem hiding this comment.
It feels a bit detached to me at the module level, but I moved it up to the function level.
|
|
||
| return [ | ||
| { [ `h${ headingLevel + 1 }` ]: `\`${ key }\`` }, | ||
| prop.description, |
There was a problem hiding this comment.
This assumes the props will always have a description. Should we add a default description here to handle if that's not the case?
There was a problem hiding this comment.
When undefined, it should be filtered out by the .filter( Boolean ) at the end.
|
Also, should we set up a way to override the docgen output, for scenarios like this one? |
Maybe? Is this prop description added manually just so we can say that it takes precedence over |
I'm actually not yet sure if that is feasible or worth the effort. Even for the docgen happening in Storybook, there are still manual steps (specifying the main component and subcomponents). And I have a hunch they are doing deeper code analysis through a webpack plugin to determine where the component source files are. I'll have a better idea about feasibility and cost–benefit once we process more components and see the amount of edge cases. |
Agreed that the best solution is to apply any change directly to the TSDoc — we could event add a custom override in our prop types, so that it gets picked up by Storybook and docgen. |
|
Addressed all feedback — ready for final review 🟢 I'll handle any remaining edge cases as I run more components through the auto-generator. |
There was a problem hiding this comment.
I don't have any blocking comment, and considering that this is meant to be the first iteration and we have plans on refining it in follow-up PRs, I'm going to approve it since @tyxla 's feedback is also addressed.
🚀
@mirka , Another suggestion for improvement: should we add a link to the component's Storybook in each README?
| manifests.map( async ( manifestPath ) => { | ||
| const manifest = await parseManifest( manifestPath ); | ||
|
|
||
| const typeDocs = getTypeDocsForComponent( { |
There was a problem hiding this comment.
getTypeDocsForComponent() is designed to throw errors, but what happens if we throw in this async function? Is anything going to handle this error?
There was a problem hiding this comment.
My intent is to immediately fail exit the entire script whenever getTypeDocsForComponent() throws, so that it's very obvious when something went wrong. Were you thinking of something else?
There was a problem hiding this comment.
Thanks for confirming, I think that works well 👍
It will be the most eye-catching thing at the top 😎 gutenberg/bin/api-docs/gen-components-docs/markdown/index.mjs Lines 14 to 17 in 0623f5b |
* Add components readme generator * Move * AlignmentMatrixControl: Add missing `@deprecated` tag * Add docs manifest for AlignmentMatrixControl * Handle case with no subcomponents * Add JSON schema * Commit AlignmentMatrixControl readme changes * Fixup: Handle case with no subcomponents * Add manifest for AnglePickerControl * Simplify * Improve schema descriptions * Handle docgen errors * Convert to async * Move glob further up * Handle unparseable JSON * Handle write file progress * Fixup * Apply feedback in markdown props handling * Simplify * Handle cases when `displayName` in manifest is wrong Co-authored-by: mirka <[email protected]> Co-authored-by: ciampo <[email protected]> Co-authored-by: tyxla <[email protected]>
Closes #58566
What?
Sets up a system to auto-generate minimal README files for components in
@wordpress/components.The readme file will include the component description and prop types information from TypeScript, as well as those for any specified subcomponents. Most importantly, it includes a visible callout at the top to nudge viewers to the Storybook docs page.
Why?
Note that the ultimate goal is not to improve the README files (and thus the documentation hosted in the Block Editor Handbook), but to increase awareness of the Storybook as a better source of component documentation. Therefore, we can keep the READMEs as simple as possible.
How?
To keep this system as low-cost as possible, I propose we start with a system where a component can opt into the auto-generator with a
docs-manifest.json. This allows us to gradually convert the existing readmes without having to build a sophisticated system upfront, with automatic handling of every edge case. We can deal with edge cases as we encounter them.What happens to content that currently only exists in the readme, like usage guidelines?
I'm planning to move this kind of extra content to a markdown file in the
storiesfolder, so they may eventually be reviewed and reused as part of the docs in Storybook. (A lot of it is probably outdated anyway, and all crucial documentation should already be in the Storybook.)Testing Instructions
Try adding a
docs-manifest.jsonto one of the components and runnpm run docs:components. (If it errors, I haven't handled that edge case yet 😇 Pick something simpler!)I have confirmed that a CI check will error if someone forgets to commit a re-generated README when they should. This has the additional benefit of acting as a basic regression test for our types — we will now know if a PR causes any unexpected changes in the public API.