docusaurus
docusaurus copied to clipboard
@theme/Tabs incorrectly types children prop
Have you read the Contributing Guidelines on issues?
- [X] I have read the Contributing Guidelines on issues.
Prerequisites
- [X] I'm using the latest version of Docusaurus.
- [ ] I have tried the
npm run clear
oryarn clear
command. - [ ] I have tried
rm -rf node_modules yarn.lock package-lock.json
and re-installing packages. - [X] I have tried creating a repro with https://new.docusaurus.io.
- [X] I have read the console error message carefully (if applicable).
Description
Discovered while swizzling the Tabs
component, it appears the type for Props['children']
is ReactElement<TabItemProps>[]
.
This is largely true, except for when the Tabs
component only has one TabItem
child, in which case it is no longer an array; it's just ReactElement<TabItemProps>
.
So we should either coerce the individual ReactElement
into an array, or update the interface to correctly reflect all valid types for children
.
Reproducible demo
https://stackblitz.com/edit/github-pv7ajd?file=src%2Ftheme%2FTabs%2Findex.tsx,docs%2Ftabs-demo.mdx
Steps to reproduce
Two ways to test this. First, open the demo link provided.
In the browser:
- With the demo open, and the dev server running (
yarn start
), navigate to/docs/tabs-demo
- Open your developer console and look for logs starting with "Is props.children an array?"
In the terminal:
- If the dev server is running, exit it
- Run
yarn build
to build the site statically - Observe the build output and look for logs starting with "Is props.children an array?"
Expected behavior
According to the type for Props["children"]
both logs should be true
.
Actual behavior
- The log for the first
Tabs
component, which has multipleTabItem
component children, istrue
. - The log for the second
Tabs
component, which has a singleTabItem
component child, isfalse
.
Your environment
No response
Self-service
- [X] I'd be willing to fix this bug myself.
I thought about that; but is there a legitimate use-case for a single tab children? I get your point that the type of props serve two purposes: (a) tell the component itself what it's going to receive at runtime (b) tell the users what they should pass to the component. Here we are trying to be restrictive for the second use-case. The point of types is to provide a strict contract and catch user mistakes as early as possible.
IMO the real problem is that tabs are mostly used in MDX where there's no type-checking, so this works just as well:
<Tabs />
But I don't think that warrants adding null
to children
, because that's not a legitimate state either. What do you think?
is there a legitimate use-case for a single tab children?
If it helps, our use-case is that we've got a page breaking down the compatibility of our software with other software, and each section can have one or more operating systems in a tabbed layout. In some cases there is only one OS. There is arguably a better approach for this, but this actually does a nice job when it comes to layout uniformity and synchronizing instructions based on the chosen OS.
I agree that this is made harder by that fact that there's no type-checking in MDX, but the important thing IMO is that it still does render when null
, a single element, or an array of them, which sets an expectation that children
should be typed as such since there is no runtime check (i.e. why I thought this was a bug).
Let's add a runtime transformation to ensure it's always an array.
I did a bit more research into this and realized that we can't transform the received children before it gets to the Tabs
component (or in my case, the wrap-swizzled version), that children being multiple types is a performance thing, and that the theme-classic component already correctly uses React.Children.map
😄 So I think the only thing to do here is to update the type - possibly from ReactElement<TabItemProps>[]
to ReactNode
?
Fixed in https://github.com/facebook/docusaurus/pull/8593