Storybook: Support proper extraction of TypeScript prop types #38842
Conversation
mirka
added
[Type] Developer Documentation
|
Size Change: +852 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
mirka
force-pushed
the
storybook/prop-type-extraction
branch
2 times, most recently
from
8387880
to
9366c7a
Compare
There was a problem hiding this comment.
Thank you @mirka , seeing this exploratory work taking shape makes very excited!
A big benefit is that the prop type documentation and all Controls can be generated automatically for the most part, with inline descriptions ✨ In some cases we may want to override or hide specific controls, but other than that we get everything for free.
This is great! It will interesting to see, as we experiment with this, which other limitations we're going to hit. In that sense, it's going to be very important for us to choose the docgen "engine" that fits our needs the most.
After a lot of experimentation, I figured out some things that were preventing
This part was very interesting to read, but it all made sense to me. This also legitimises the priority that we should give to #35744
we can expect that most consumer devs will want to interact with Storybook primarily through the Docs view
Yes 🚀
Eventually, I imagine we'll want to make the Docs mode version of our Storybook to be our canonical documentation, instead of the limited Component Reference currently on the dotorg site.
...
So whenever possible, it'll be beneficial for us to write TypeScript and Storybook stories in a way that makes sense for the Docs view.
That's what I have in mind, too.
Are there any weird props that show up in Docs view that you want to hide? Consider whether it should be Omit-ed at the TypeScript level. (Example: #38844)
Specifically on the as prop, I think it'd be good to expose it, since some components are polymorphic, while others are not. WDYT ?
Are some stories showing unhelpful code snippets in Docs view? See if you can tweak the story to show an understandable code sample.
This will probably need a story-by-story "human" review.
Maybe a good balance for our story examples could be:
- Default story: expose as many props as possible, so that a dev can tweak them while reading the docs
- Non-default stories: more specific example/scenarios, with very few controls exposed
How much work is left in order to get this extraction to a "good enough" point? Woudl it require all components and Storybook examples to be migrated to TypeScript first?
Right. FWIW, out of the two docgens that Storybook supports,
Yes, it will be present for polymorphic components, and omitted from non-polymorphic components, if that’s what you’re asking 👍
The thing is, all props recognized by the docgen will automatically be exposed as Controls on all stories by default. At that point, if we want to hide any Controls we have to disable each one specifically (denylist). This is how Storybook works, and it’s not something we can easily change to be an allowlist system. (The only reason we weren’t getting this behavior is because the majority of our types are currently unrecognizable by the docgens 😅) So in most cases I don’t think it is worth the effort to carefully curate the set of Controls exposed on specific stories. This might seem like an annoyance, but ultimately I think the behavior is sensible, and informs us how we should be approaching the different Storybook sections. The Docs view code snippet is for understanding which props are being explicitly set. The Canvas view is for advanced tinkering and debugging, in which case we actually do want to expose as many Controls as possible. Is that reasonable?
I think we can approach it in a “progressive enhancement” way. First off we can merge this PR, since it doesn’t have ill effects on any existing stories. Then every time a TS conversion is complete, we can tweak the stories for that component so it makes use of the type data. Once the majority of TS conversions are complete, we can set the default tab to Docs. Once 100% of the TS conversions are complete, we can remove markdown docs and point everyone to the Storybook. |
That's good to know. Out of curiosity, what does
Yup, awesome!
Yes, thank you for the explanation. I guess we should stil add multiple stories to the Canvas view showcasing specific scenarios / recipes?
Sounds like a plan to me. I will give this PR a final review then! |
mirka
force-pushed
the
storybook/prop-type-extraction
branch
from
ee1922b
to
a556b52
Compare
There was a problem hiding this comment.
Apart from the previous comment and the inline comments, here's some additional thoughts:
- Is there a way to sort the props displayed in the docs alphabetically? When there's a lot props, it may help to have a way of sorting them so that a user can easily find them (Ideally we could even group them, but it may be a task that we want to tackle separately)
- A lot of props can't be expanded to their "raw" value (e.g the
adjustLineHeightForInnerControlsprop ofHeadinghas a type ofTextAdjustLineHeightForInnerControls). Although this is not great, the opposite approach (i.e expanding every alias) could be even worse. Probably better for now to keep the alias expansion deactivated? - When guessing a prop type for the associated control, Storybook seems to default to
Object, which is often not the right choice (this happens, for example, for all CSS-related props) and can undermine the usefulness of the docs. Probably the only solution here is to manually override those control types for each story that gets converted? - As a special case of the previous point, the
asprop is also presented as an Object. Clicking on the "Set object" button currently crashes Storybook - We should also look into adding a description for the
asprop in theWordPressComponenttyperybook defaults to an Object type, which breaks storybook
And finally, some additional feedback specific to the Heading and Divider stories:
- Heading: not something introduced by this PR, but exposing the component through the new story revealed quite a few unpredictable (and very likely unintended) behaviors, especially around tweaking character-based and line-based truncation. Also, the
displayprop value always gets overridden by a `display: block' rule declared later - Divider: in the default "Horizontal" story (which is used to highlight all
Dividerprops), the user is able to selectverticalfor the orientation, but currently the story doesn't really support that use case (i.e. theDividercan't be seen). We should probably:- Update the story by adding a flex wrapper
- Update the docs of the
orientationprop
Export unconnected `Heading`
This is required for the code snippets to show the correct component name.
`requiredFirst` seems to also sort the rest of the props alphabetically 🎉
mirka
force-pushed
the
storybook/prop-type-extraction
branch
from
a556b52
to
354c730
Compare
|
I think we're in a pretty good place, thanks to your feedback! All issues addressed.
I don’t have much information aside from the comparison table, but apparently it’s faster and has fewer bugs, albeit with lower quality type analysis? Both repos seem reasonably active.
Exactly! When there are a lot of items being exposed in the props table, it’s important that we showcase the common usage examples with stories.
Done ✅ Required props first, then the rest are alphabetical.
Yes, also I think another way of approaching it in certain cases is to actually not use a type alias. Like in your example,
Currently, yes. Let’s see how often it happens. FWIW, there is a already a basic mechanism to regex-match and map props to certain controls. It doesn’t support our needs yet, but the code looks pretty straightforward so it might be a good place to contribute. (I went ahead and upvoted in an upstream feature request.)
Fixed ✅
Done ✅
|
There was a problem hiding this comment.
Basically, a solid 👍 to all of your answers above 🚀
I left a few inline comments, but I am preentively approving this PR as they are all small changes — feel free to merge after resolving them if you think so!
Also, 👍 re. the proposed follow-ups:
- Adding https://github.com/izhan/storybook-description-loader
- Iterating on
Divider's story (especially around the vertical orientation)
| adjustLineHeightForInnerControls?: | ||
| | boolean | ||
| | 'large' | ||
| | 'medium' | ||
| | 'small' | ||
| | 'xSmall'; |
There was a problem hiding this comment.
Maybe the "limit" for expanding type aliases is if the types that are unioned are all of the same general type or not?
E.g. a union of just strings could be "easier" to expand for the docgen, while a union of strings and boolean may be kept un-expanded?
There was a problem hiding this comment.
Me neither, but good to keep at the back of our minds for future work :)
| letterSpacing: { control: { type: 'text' } }, | ||
| lineHeight: { control: { type: 'text' } }, | ||
| optimizeReadabilityFor: { control: { type: 'color' } }, | ||
| variant: { control: { type: 'radio' }, options: [ 'muted' ] }, |
There was a problem hiding this comment.
Nit: using a radio with only one option, it's not possible to "deselect" that option, once selected
There was a problem hiding this comment.
Kind of like the "reset" problem in the editor sidebar! I actually think this is consistent with all controls (especially auto-generated ones), in that you can't really go back to the undefined state once you set a value. We could of course add an undefined option, but I think the overall design intent is to use the "Reset controls" button ↩️ in the top right.
I'm not sure how I feel about this yet! On one hand I don't want to establish a pattern of adding a lot of manual tweaks to auto-generated controls, because it increases maintenance burden. But in some cases it may be annoying to have to hit Reset each time. So... maybe worth it for heavily trafficked props like variant on Button?
There was a problem hiding this comment.
Agreed. Let's leave it as-is (and rely on the reset button for now). I believe we are amongst the heavy users of this Storybook anyway, so we can make this change if we ever feel like it's really needed.
| component: Heading, | ||
| title: 'Components (Experimental)/Heading', | ||
| argTypes: { | ||
| adjustLineHeightForInnerControls: { control: { type: 'text' } }, |
There was a problem hiding this comment.
adjustLineHeightForInnerControls can also be a boolean — should we add true and false to the list of possible values, maybe in a dropdown-like control?
There was a problem hiding this comment.
Mmm yeah, this one’s a bit overcomplicated too. Based on the code, true gives the same result as 'medium', and false gives the same result as undefined. Before we graduate <Text> from experimental status, I'd prefer removing the boolean types from this prop API. (Kind of similar to your UnitControl cleanup!)
I'll note this in the TS refactor tracking issue.
There was a problem hiding this comment.
Just to close the circle, this was finally done in #54953 !
| letterSpacing: { control: { type: 'text' } }, | ||
| lineHeight: { control: { type: 'text' } }, |
There was a problem hiding this comment.
These values can be both strings or numbers, is there a way to achieve that with controls?
There was a problem hiding this comment.
The object control can strictly cover both, since that's basically a JSON string input so it differentiates between 1 and "1". Though in the vast majority of cases I think a normal text control would be sufficient for tinkering and better for UX (because people will have trouble recognizing that they need to quote their strings in a object control).
Description
This PR clears the path for proper extraction of component prop types to use in our Storybook and documentation. Given our ongoing investment in TypeScript migrations, it would be great to leverage this as much as possible.
I tweaked two components here (
HeadingandDivider) to demonstrate what it could look like. A big benefit is that the prop type documentation and all Controls can be generated automatically for the most part, with inline descriptions ✨ In some cases we may want to override or hide specific controls, but other than that we get everything for free.Details
After a lot of experimentation, I figured out some things that were preventing Storybook's
react-docgen-typescriptfrom properly extracting props information from our TypeScript/JSDocs. Here's what's required for proper type extraction:WordPressComponentPropstype alias needs to be imported from a proper.tssource file. The problem was that our components were importing it viaui/context/index.js, which is not a.tsfile.contextConnect()HOC needs to be in TypeScript.default.What this means for TypeScript migration and story authoring
As more components are converted to TypeScript and their stories are improved, we can expect that most consumer devs will want to interact with Storybook primarily through the Docs view, rather than the Canvas. Especially with code samples (92354c9) visible for each story, it's much easier to understand how to use the components. And for the default story, the Controls are available right in the Docs view. Canvas view will only be necessary for advanced debugging.
Eventually, I imagine we'll want to make the Docs mode version of our Storybook to be our canonical documentation, instead of the limited Component Reference currently on the dotorg site.
So whenever possible, it'll be beneficial for us to write TypeScript and Storybook stories in a way that makes sense for the Docs view. For example:
Omit-ed at the TypeScript level. (Example: Context: Omitasprop in types #38844)Testing Instructions
npm run storybook:devHeadingandDivider. Compare with the ones on https://wordpress.github.io/gutenberg/npx start-storybook -c ./storybook -p 50240 --docsto see how it would look like in Docs mode.Screenshots
CleanShot.2022-02-16.at.09.10.25-converted.mp4
Types of changes
.ts, re-exporting as named components)Checklist:
*.native.jsfiles for terms that need renaming or removal).