fix(render): Don't reject wrapper types based on statics #973
Conversation
eps1lon
added
bug
| options?: pure.RenderOptions, | ||
| ) { | ||
| const Wrapper: React.FunctionComponent = ({children}) => { | ||
| const Wrapper: React.FunctionComponent<{children?: React.ReactNode}> = ({ |
There was a problem hiding this comment.
While FunctionComponent adds implicit children, its propTypes do not so we didn't catch that issue. Here we make sure all statics assume the same props. Also makes sure this doesn't break once we get rid of implicit children in React.FunctionComponent.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 54057ef:
|
Codecov Report
@@ Coverage Diff @@
## main #973 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 140 140
Branches 26 26
=========================================
Hits 140 140 Continue to review full report at Codecov.
|
|
🎉 This PR is included in version 12.1.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
| * @see https://testing-library.com/docs/react-testing-library/api/#wrapper | ||
| */ | ||
| wrapper?: React.ComponentType<{children: React.ReactElement}> | ||
| wrapper?: React.JSXElementConstructor<{children: React.ReactElement}> |
There was a problem hiding this comment.
I'm wondering whether we should define children as React.ReactNode which seems to be the "official" type for children props. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/react/index.d.ts#L829
There was a problem hiding this comment.
wrapper never receives anything other than ReactElement. If we would widen children to ReactNode, the implementation of wrapper would have to account for types that it never actually receives.
This is problematic if you want to apply methods to children in the implementation of wrapper that require a single ReactElement such as React.cloneElement.
There was a problem hiding this comment.
That makes perfect sense - ReactNode is indeed very wide. Thanks for the explanation.
|
🎉 This PR is included in version 13.0.0-alpha.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
What:
Closes #970
Closes #972
Why:
Broke in #966
How:
Implicit children strikes yet again. While
FunctionComponentadds implicit children, itspropTypesdo not so we didn't catch that issue. We don't care about React statics and just that we can pass it tocreateElement(or any JSX runtime).Checklist:
[ ]Documentation added to thedocs site