-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(TS): make wrapper allow a simple function comp #966
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
I don't like merging my own PRs, especially in projects I'm not actively maintaining 😅 So I can wait until someone else can get around to this one. Thanks! |
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 f23919d:
|
Codecov Report
@@ Coverage Diff @@
## main #966 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 140 140
Branches 26 26
=========================================
Hits 140 140 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 🚀
types/index.d.ts
Outdated
wrapper?: React.ComponentType | ||
wrapper?: | ||
| React.ComponentType | ||
| ((props: {children: React.ReactNode}) => JSX.Element) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me add a type test for this because the fix is simpler I believe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implicit children strikes again (DefinitelyTyped/DefinitelyTyped#34237):
React.ComponentType
includes React.FunctionComponent
which means the accepted props are { children?: React.ReactNode }
. However, our Wrapper
only accepts { children: React.ReactElement }
props. So we could render <Wrapper>Some Child</Wrapper>
which may not be implemented by the custom Wrapper
component because now it receives { children: string }
.
The proposed fix would not have fixed the linked issue (npm typecheck
fails on ffa2221). dc03098 fixes the linked issue.
LGTM 👍 Thanks! |
🎉 This PR is included in version 12.1.1 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
`@testing-library/react` introduced new type definition for `wrapper` in PR testing-library/react-testing-library#966, then released it in `v12.1.1`. issue testing-library/react-testing-library#970
`@testing-library/react` introduced new type definition for `wrapper` in PR testing-library/react-testing-library#966, then released it in `v12.1.1`. issue testing-library/react-testing-library#970 5420890
Hi @eps1lon! We currently have code that, once contrived a lot for the sake of sharing an example, looks somewhat like this: import { PropsWithChildren } from 'react';
function Foo({ children }: PropsWithChildren<Bar>) {
return (
options?.wrapper ? (
<options.wrapper>{children}</options.wrapper>
) : (
children
)
);
}
type PropsWithChildren<P> = P & { children?: ReactNode | undefined }; This worked fine with
What would be your recommendation here? Should we simply change the code above into (It seems this discussion with @drzamich is also relevant: https://github.com/testing-library/react-testing-library/pull/973/files#r754242764) Thank you! ❤️ |
🎉 This PR is included in version 13.0.0-alpha.7 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What: This adds an option for the wrapper to use a simple function component
Why: Closes #965
How: Added the option to the types for
render
Checklist:
docs site N/A