-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Components: Set up README auto-generator #66035
Changes from 10 commits
f31de7c
82cafc7
04f28a7
bbdf0fb
a0d09d0
5a524a3
b363002
5d69c1b
680a420
1d01ecf
9bf7678
8fe2ca8
7c66c58
38b6d34
60f9cc9
eb4cb40
d2a2487
752792d
6fbf877
0623f5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
const docgen = require( 'react-docgen-typescript' ); | ||
const glob = require( 'glob' ); | ||
const fs = require( 'fs' ); | ||
const path = require( 'path' ); | ||
|
||
/** | ||
* Internal dependencies | ||
*/ | ||
const { generateMarkdownDocs } = require( './markdown' ); | ||
|
||
// For consistency, options should generally match the options used in Storybook. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a way to read those same options from the storybook config, instead of duplicating them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately they're internal. I just pulled them from the source code. |
||
const OPTIONS = { | ||
shouldExtractLiteralValuesFromEnum: true, | ||
shouldRemoveUndefinedFromOptional: true, | ||
propFilter: ( prop ) => | ||
prop.parent ? ! /node_modules/.test( prop.parent.fileName ) : true, | ||
savePropValueAsString: true, | ||
}; | ||
|
||
function getTypeDocsForComponent( { | ||
manifestPath, | ||
componentFilePath, | ||
displayName, | ||
} ) { | ||
const resolvedPath = path.resolve( | ||
path.dirname( manifestPath ), | ||
componentFilePath | ||
); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we bail here if the path isn't resolved? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I combined the error handling into a single throw with any other potential |
||
|
||
return docgen | ||
.parse( resolvedPath, OPTIONS ) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not that I know of, but we'll see when we run more components through the system. |
||
.find( ( obj ) => obj.displayName === displayName ); | ||
} | ||
|
||
const manifests = glob.sync( 'packages/components/src/**/docs-manifest.json' ); | ||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
manifests.forEach( ( manifestPath ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There could be an opportunity to process multiple of these sync tasks in parallel and make this run faster. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Converted the per-file processing to async 👍 |
||
const manifest = JSON.parse( fs.readFileSync( manifestPath, 'utf8' ) ); | ||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const typeDocs = getTypeDocsForComponent( { | ||
manifestPath, | ||
componentFilePath: manifest.filePath, | ||
displayName: manifest.displayName, | ||
} ); | ||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const subcomponentTypeDocs = manifest.subcomponents?.map( | ||
( subcomponent ) => { | ||
const docs = getTypeDocsForComponent( { | ||
manifestPath, | ||
componentFilePath: subcomponent.filePath, | ||
displayName: subcomponent.displayName, | ||
} ); | ||
|
||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if ( subcomponent.preferredDisplayName ) { | ||
docs.displayName = subcomponent.preferredDisplayName; | ||
} | ||
|
||
return docs; | ||
} | ||
); | ||
|
||
const docs = generateMarkdownDocs( { typeDocs, subcomponentTypeDocs } ); | ||
const outputFile = path.resolve( | ||
path.dirname( manifestPath ), | ||
'./README.md' | ||
); | ||
|
||
fs.writeFileSync( outputFile, docs ); | ||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} ); |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,44 @@ | ||||||||
/** | ||||||||
* External dependencies | ||||||||
*/ | ||||||||
const json2md = require( 'json2md' ); | ||||||||
|
||||||||
/** | ||||||||
* Internal dependencies | ||||||||
*/ | ||||||||
const { generateMarkdownPropsJson } = require( './props' ); | ||||||||
|
||||||||
function generateMarkdownDocs( { typeDocs, subcomponentTypeDocs } ) { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any good reason those are passed as an object and not as separate arguments? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just that we might want to pass more arguments to this function later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need some checks if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added a bit more safeguards, which I think should be sufficient for now. |
||||||||
const mainDocsJson = [ | ||||||||
'<!-- 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have similar callouts for private, deprecated, and experimental components? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. And what about deprecated? Same strategy as experimental? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||||
}, | ||||||||
typeDocs.description, | ||||||||
...generateMarkdownPropsJson( typeDocs.props ), | ||||||||
]; | ||||||||
|
||||||||
const subcomponentDocsJson = subcomponentTypeDocs | ||||||||
? [ | ||||||||
{ h2: 'Subcomponents' }, | ||||||||
...subcomponentTypeDocs.flatMap( ( subcomponentTypeDoc ) => [ | ||||||||
{ | ||||||||
h3: subcomponentTypeDoc.displayName, | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we convinced There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I know, it should be sufficient for now. |
||||||||
}, | ||||||||
subcomponentTypeDoc.description, | ||||||||
...generateMarkdownPropsJson( subcomponentTypeDoc.props, { | ||||||||
headingLevel: 4, | ||||||||
} ), | ||||||||
] ), | ||||||||
] | ||||||||
: []; | ||||||||
|
||||||||
return json2md( | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||||||||
[ ...mainDocsJson, ...subcomponentDocsJson ].filter( Boolean ) | ||||||||
); | ||||||||
} | ||||||||
|
||||||||
module.exports = { | ||||||||
generateMarkdownDocs, | ||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,53 @@ | ||
function renderPropType( type ) { | ||
switch ( type.name ) { | ||
case 'enum': { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any other prop types to handle? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe. I'll add more logic as I encounter them. |
||
const MAX_ENUM_VALUES = 10; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this const be declared up there at the module level? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It feels a bit detached to me at the module level, but I moved it up to the function level. |
||
const string = type.value | ||
.slice( 0, MAX_ENUM_VALUES ) | ||
.map( ( { value } ) => value ) | ||
.join( ' | ' ); | ||
|
||
if ( type.value.length > MAX_ENUM_VALUES ) { | ||
return `${ string } | ...`; | ||
} | ||
return string; | ||
} | ||
default: | ||
return type.name; | ||
} | ||
} | ||
|
||
function generateMarkdownPropsJson( props, { headingLevel = 2 } = {} ) { | ||
const sortedKeys = Object.keys( props ).sort( ( [ a ], [ b ] ) => | ||
a.localeCompare( b ) | ||
); | ||
|
||
const propsJson = sortedKeys | ||
.flatMap( ( key ) => { | ||
const prop = props[ key ]; | ||
|
||
if ( prop.description.includes( '@ignore' ) ) { | ||
mirka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return null; | ||
} | ||
|
||
return [ | ||
{ [ `h${ headingLevel + 1 }` ]: `\`${ key }\`` }, | ||
prop.description, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When undefined, it should be filtered out by the |
||
{ | ||
ul: [ | ||
`Type: \`${ renderPropType( prop.type ) }\``, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For displaying the prop type, I opted to use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
`Required: ${ prop.required ? 'Yes' : 'No' }`, | ||
prop.defaultValue && | ||
`Default: \`${ prop.defaultValue.value }\``, | ||
].filter( Boolean ), | ||
}, | ||
]; | ||
} ) | ||
.filter( Boolean ); | ||
|
||
return [ { [ `h${ headingLevel }` ]: 'Props' }, ...propsJson ]; | ||
} | ||
|
||
module.exports = { | ||
generateMarkdownPropsJson, | ||
}; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
{ | ||
"title": "JSON schema for @wordpress/components README manifests", | ||
"$schema": "http://json-schema.org/draft-07/schema#", | ||
"type": "object", | ||
"properties": { | ||
"displayName": { | ||
"type": "string", | ||
"description": "The `displayName` of the component, as determined in code." | ||
}, | ||
"filePath": { | ||
"type": "string", | ||
"description": "The file path where the component is located." | ||
}, | ||
"subcomponents": { | ||
"type": "array", | ||
"description": "List of subcomponents related to the component.", | ||
"items": { | ||
"type": "object", | ||
"properties": { | ||
"displayName": { | ||
"type": "string", | ||
"description": "The `displayName` of the subcomponent, as determined in code." | ||
}, | ||
"preferredDisplayName": { | ||
"type": "string", | ||
"description": "The display name to use in the README, if it is different from the `displayName` as determined in code." | ||
}, | ||
"filePath": { | ||
"type": "string", | ||
"description": "The file path where the subcomponent is located." | ||
} | ||
}, | ||
"required": [ "displayName", "filePath" ] | ||
} | ||
} | ||
}, | ||
"required": [ "displayName", "filePath" ] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.