React < 18: Fix incorrect ReactNode declaration
#263
Conversation
| @@ -77,6 +77,8 @@ const invalidParamsWithGeneric: Params<{ id: number }> = { id: 13 }; // $ExpectE | |||
| {({ rest }) => { | |||
| const fn = (a: string) => "noop"; | |||
| fn(rest); // $ExpectError | |||
|
|
|||
| return <></>; | |||
There was a problem hiding this comment.
The previous definition was incorrect, but thanks to the fix I was able to catch it.
size-limit report 📦
|
Codecov ReportBase: 100.00% // Head: 100.00% // No change to project coverage 👍
Additional details and impacted files@@ Coverage Diff @@
## master #263 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 7 7
Lines 255 255
Branches 50 50
=========================================
Hits 255 255 Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
| @@ -59,7 +59,7 @@ const invalidParamsWithGeneric: Params<{ id: number }> = { id: 13 }; // $ExpectE | |||
| This is a <b>mixed</b> content | |||
| </Route>; | |||
|
|
|||
| <Route path="/users/:id">{(params: Params): React.ReactNode => `User id: ${params.id}`}</Route>; | |||
| <Route path="/users/:id">{(params: Params) => `User id: ${params.id}`}</Route>; | |||
There was a problem hiding this comment.
This one is interesting. Because we use React 17 for testing the types, the two ReactNode versions were conflicting with each other.
This could lead to some cases where a user decides to define their function explicitly returning ReactNode and they might run into this issue. But I suppose since it was a bug they, unfortunately, would have to fix it themselves.
| @@ -23,6 +24,11 @@ import { DefaultParams, Params, Match, MatcherFn } from "../matcher"; | |||
| export * from "../matcher"; | |||
| export * from "../use-location"; | |||
|
|
|||
| // React <18 only: fixes incorrect `ReactNode` declaration that had `{}` in the union. | |||
There was a problem hiding this comment.
Side thought: honestly, supporting two versions of TS becomes a bit of a burden. I wonder if we could drop support for 3.9.4 in the future major version. Would that be too brutal? Imo, it could, but there is an alternative – can't upgrade to TS 4, well you would have to stay on wouter@2, sorry.
There was a problem hiding this comment.
🤷♂️ I have no idea. Personally, I don't use TS < 4 and never plan to. But others may have a different opinion on this 😄
|
Hi @HansBrende, please see the solution provided. |
types/ts3.9.4/index.d.ts
Outdated
| // React <18 only: fixes incorrect `ReactNode` declaration that had `{}` in the union. | ||
| // This issue has been fixed in React 18 type declaration. | ||
| // https://github.com/DefinitelyTyped/DefinitelyTyped/pull/56210 | ||
| type ReactNode = ReactChild | ReactNode[] | ReactPortal | boolean | null | undefined; |
There was a problem hiding this comment.
Looks good. Only other thing I will suggest is to change
ReactNode[] to Iterable<ReactNode>. This was a change in DT that I overlooked. See details here: DefinitelyTyped/DefinitelyTyped#56623
Also worth noting, react has allowed Iterables prior to 18, it was always just incorrectly typed in DT (a problem which was masked by the inclusion of {}). https://github.com/facebook/react/blob/v17.0.2/packages/shared/ReactTypes.js#L20
There was a problem hiding this comment.
Good one! Fixed in a3c480e
|
Thank you @HansBrende, I have just published |
Closes #229
This PR declares more strict
ReactNodethat excludes{}. Only relevant to React.It also fixes some incorrect test cases.