Skip to content
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

fix(TS): make wrapper allow a simple function comp #966

Merged
merged 7 commits into from Sep 27, 2021
Merged

Conversation

kentcdodds
Copy link
Member

@kentcdodds kentcdodds commented Sep 24, 2021

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:

  • Documentation added to the
    docs site N/A
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

@kentcdodds
Copy link
Member Author

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!

@codesandbox-ci
Copy link

codesandbox-ci bot commented Sep 24, 2021

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:

Sandbox Source
React Configuration
react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Sep 24, 2021

Codecov Report

Merging #966 (206757b) into main (a218b63) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

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

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a218b63...206757b. Read the comment docs.

3imed-jaberi
3imed-jaberi previously approved these changes Sep 25, 2021
Copy link

@3imed-jaberi 3imed-jaberi left a 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)
Copy link
Member

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.

Copy link
Member

@eps1lon eps1lon Sep 27, 2021

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.

@kentcdodds
Copy link
Member Author

LGTM 👍 Thanks!

@eps1lon eps1lon merged commit cde904c into main Sep 27, 2021
@eps1lon eps1lon deleted the pr/wrapper-types branch September 27, 2021 12:39
@github-actions
Copy link

🎉 This PR is included in version 12.1.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

sabertazimi added a commit to sabertazimi/blog that referenced this pull request Oct 1, 2021
`@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
github-actions bot added a commit to sabertazimi/blog that referenced this pull request Oct 1, 2021
`@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
@astorije
Copy link

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
    )
  );
}

PropsWithChildren comes from @types/react and is simply:

type PropsWithChildren<P> = P & { children?: ReactNode | undefined };

This worked fine with @testing-library/react v12.1.0, but starting in v12.1.2 we're seeing the following, after the type change in this PR:

error TS2322: Type 'ReactNode' is not assignable to type 'ReactElement<any, string | JSXElementConstructor<any>>'.
  Type 'undefined' is not assignable to type 'ReactElement<any, string | JSXElementConstructor<any>>'.

             <options.wrapper>{children}</options.wrapper>

What would be your recommendation here?

Should we simply change the code above into <options.wrapper><>{children}</></options.wrapper>?

(It seems this discussion with @drzamich is also relevant: https://github.com/testing-library/react-testing-library/pull/973/files#r754242764)

Thank you! ❤️

@github-actions
Copy link

🎉 This PR is included in version 13.0.0-alpha.7 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Render Function Wrapper Typescript Issue
4 participants