❌ Deploy Preview forreact-hooks-testing-library failed.

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 forReactNode, 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.

Minor bump for review—anyone?

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

Yes. We're using@types/react@18 andreact@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.

Yes. We're using@types/react@18 andreact@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 tochildren in the 18 update of the type lib iirc.

Codecov Report

Merging#848 (291945d) intomain (e2461ca) willnot change coverage.
The diff coverage isn/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 byCodecov. Last updatee2461ca...291945d. Read thecomment docs.

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 tochildren 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 usinguseCallback, no implicitchildren (which this PR addresses), strictReactNode... 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 implicitchildren 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 thatchildren always statically exists.

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 toreact18 at all, if you want to test your hooks withr18 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.

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 bypierrec/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 implicitchildren 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.