@theme/Tabs incorrectly types children prop

[jodyheavener]

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 or yarn 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 multiple TabItem component children, is true.
• The log for the second Tabs component, which has a single TabItem component child, is false.

Your environment

No response

Self-service

• [X] I'd be willing to fix this bug myself.

[Josh-Cena]

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?

[jodyheavener]

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).

[Josh-Cena]

Let's add a runtime transformation to ensure it's always an array.

[jodyheavener]

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?

[jodyheavener]

Fixed in https://github.com/facebook/docusaurus/pull/8593