fix: avoid relying on implicit children prop

[Josh-Cena]

What:

This PR adds children: ReactNode to the props of WrapperComponent.

Why:

Docusaurus is doing something a bit weird: we are using React 17, but at the same time, we use @types/react@18 to enjoy its stricter types. The only thing hindering it from working properly is that WrapperComponent does not declare that it will always pass the children prop. This leads to type errors in test files:

wrapper: ({children}) => (
^^^^^^^
Type '({ children }: { children: any; }) => Element' is not assignable to type 'WrapperComponent<unknown>'.
  Type '({ children }: { children: any; }) => Element' is not assignable to type 'FunctionComponent<unknown>'.
    Types of parameters '__0' and 'props' are incompatible.
      Property 'children' is missing in type '{}' but required in type '{ children: any; }'.

How:

Just added an explicit children prop.

Checklist:

• Documentation updated
• Tests
• Ready to be merged
  
• Added myself to contributors table
  

[netlify]

❌ Deploy Preview for react-hooks-testing-library failed.

Name	Link
🔨 Latest commit	291945d
🔍 Latest deploy log	https://app.netlify.com/sites/react-hooks-testing-library/deploys/62b56f690b3537000877f69c

[Josh-Cena]

Sigh Getting some type-checking errors.

'ErrorBoundary' cannot be used as a JSX component.
  Its instance type 'ErrorBoundary' is not a valid JSX element.
    The types returned by 'render()' are incompatible between these types.
      Type 'React.ReactNode' is not assignable to type 'import("react-hooks-testing-library/node_modules/@types/react-test-renderer/node_modules/@types/react/index").ReactNode'.
        Type '{}' is not assignable to type 'ReactNode'.

@types/react-test-renderer is pulling in @types/react 18, which has a stricter type for ReactNode, but there's no way to force resolution with npm. I don't know if this would mask away other type errors with this PR, but the change looks innocent to me.

[Josh-Cena]

Minor bump for review—anyone?

[joshuaellis]

Sorry, just so i'm clear, you're using react18 types on your project and this is causing your error?

[Josh-Cena]

Yes. We're using @types/react@18 and react@17, to be clear. It isn't very critical because test files are type-checked separately and don't block build, but this still creates a bad DX.

[joshuaellis]

Yes. We're using @types/react@18 and react@17, to be clear. It isn't very critical because test files are type-checked separately and don't block build, but this still creates a bad DX.

Maybe i'm misunderstanding so apologies, but this lib doesn't support react18, so wouldn't it be fair to say that this issue is a mismatch of dependencies? i.e. all our types incl. exported ones are based on react17 types and there were some major changes to children in the 18 update of the type lib iirc.

[codecov]

Codecov Report

Merging #848 (291945d) into main (e2461ca) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##              main      #848   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           15        15           
  Lines          245       245           
  Branches        34        34           
=========================================
  Hits           245       245           

---

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 e2461ca...291945d. Read the comment docs.

[Josh-Cena]

Maybe i'm misunderstanding so apologies, but this lib doesn't support react18, so wouldn't it be fair to say that this issue is a mismatch of dependencies? i.e. all our types incl. exported ones are based on react17 types and there were some major changes to children in the 18 update of the type lib iirc.

Indeed, that's why I said we are probably doing something weird here, but so far everything is working as expected. React 18 doesn't introduce breaking runtime API changes (and I doubt they will in any future major version), so upgrading the types package is a net gain, because of stricter types like no implicit any when using useCallback, no implicit children (which this PR addresses), strict ReactNode... All of which help us ship higher-quality types and more runtime-safe code.

The library would likely need to do it anyway if we every plan to be compatible with React 18. (I don't know if that's a goal; I did see the README notice but I can't fully appreciate its implications.) Reliance on implicit children prop has never been an encouraged practice, because it makes the API surface quite implicit and break-prone, and especially in this case where there's an inversion of control and the wrapper is in fact a callback, it's better to be explicit that children always statically exists.

[joshuaellis]

The library would likely need to do it anyway if we every plan to be compatible with React 18. (I don't know if that's a goal; I did see the README notice but I can't fully appreciate its implications.)

We're not upgrading to react18 at all, if you want to test your hooks with r18 you should use @testing-library/react. So I don't think upgrading the types should be encouraged either, it sends the wrong message for the library.

The tests seem to pass which is a good start. Think the docs fail due to something else.

I don't necessarily have an issue with this PR although i'd have to let @mpeyper have final say.

[Josh-Cena]

Think the docs fail due to something else.

The docs fail because the Netlify deployment is using an outdated version of Node. This is caused by pierrec/node-eval#28. (Yes, I know it because I implemented that as well 😄) The lack of a lockfile (which I can emphasize with) combined with that PR shipped in a patch version causes this. (It was rightfully a patch because Node 10 was long EOL'ed at that time)

it sends the wrong message for the library.

The implicit children was a half-bug-half-feature in the first place and is never encouraged to be relied on—if not because it's such a popular package it would probably have been fixed in a minor. We are just promoting better practice, which @types/react@18 happens to strictly enforce.