feat(Tabs): enforce restricted type for children #6767
Conversation
jonkoops
force-pushed
the
enforce-tabs-children
branch
from
1ca97d2 to
aecdce3
Compare
| /** Content rendered inside the tabs component. Must be React.ReactElement<TabProps>[] */ | ||
| children: React.ReactNode; | ||
| /** Content rendered inside the tabs component. */ | ||
| children: Child | Child[]; |
There was a problem hiding this comment.
The way our docs are generated from the props interface, the website is not doing much to describe what the definition of Child is. (see here)
The easiest way to fix this for now would probably be to be more descriptive about the type def of Child in the prop description comment.
There was a problem hiding this comment.
@nicolethoen I've pushed a commit to clarify this a bit WDYT?
| /** Content rendered inside the tabs component. Must be React.ReactElement<TabProps>[] */ | ||
| children: React.ReactNode; | ||
| /** Content rendered inside the tabs component. */ | ||
| children: Child | Child[]; |
There was a problem hiding this comment.
Technically this is a breaking change. It probably would not impact anyone but we try and not introduce anything that could be breaking like more restrictive type changes. We can pull this update into our next breaking change release.
There was a problem hiding this comment.
Based on the issue - it sounds like this type enforcement is already necessary - using any other type would already produce a broken tab
There was a problem hiding this comment.
Although the type change is technically breaking, the runtime behavior is actually improved. Before if you'd pass in a truthy value the component would attempt to render it, and since there are no props associated with these children this will cause a run-time error (see this example).
This is caused by the fact that before the children were filtered using a boolean filter, which would allow truthy values that are not React elements. In this PR these filters are replaced by isValidElement(), which asserts the value passed is a valid React element created with createElement(), meaning the required props will be present.
So considering that there could realistically not be any applications out there doing this, unless they created a new component that matches all of the required props of Tab I don't see this as a breaking change.
There was a problem hiding this comment.
@jonkoops I totally agree. I was thinking worst case scenario like you mention in your last point. If they create a component that matches all of the required props of Tab. Highly unlikely though.
|
@nicolethoen @tlabaj do we want to go ahead and merge this one? (I don't have permission to do this) |
|
as soon as @tlabaj gives this a thumbs up, she can merge it :) |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
|
@nicolethoen could you re-open this issue as it's solution was reverted? |
|
I did reopen the associated issue: #6766 |
|
Thanks! |
Closes #6766