-
Notifications
You must be signed in to change notification settings - Fork 232
fix: avoid relying on implicit children prop #848
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
❌ Deploy Preview for react-hooks-testing-library failed.
|
52dcc43
to
c11cefd
Compare
c11cefd
to
37f1328
Compare
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.
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
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.
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 to children
in the 18 update of the type lib iirc.
Codecov Report
Merging #848 (291945d) into main (e2461ca) will not 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 by Codecov. Last update e2461ca...291945d. Read the comment 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 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.
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
What:
This PR adds
children: ReactNode
to the props ofWrapperComponent
.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 thatWrapperComponent
does not declare that it will always pass thechildren
prop. This leads to type errors in test files:How:
Just added an explicit
children
prop.Checklist: