Updated with explicit children in mind #510
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
eps1lon
force-pushed
the
06-04-Updated_with_explicit_children
branch
from
409e889
to
4db5b44
Compare
eps1lon
commented
Jun 4, 2022
Comment on lines
-62
to
-67
| children1: JSX.Element; // bad, doesnt account for arrays | ||
| children2: JSX.Element | JSX.Element[]; // meh, doesn't accept strings | ||
| children3: React.ReactChildren; // despite the name, not at all an appropriate type; it is a utility | ||
| children4: React.ReactChild[]; // better, accepts array children | ||
| children: React.ReactNode; // best, accepts everything (see edge case below) | ||
| functionChildren: (name: string) => React.ReactNode; // recommended function as a child render prop type |
There was a problem hiding this comment.
There's no sensible recommendation for render prop type since it depends heavily on what you want to do. The pattern itself is rarely used nowadays so let's not overwhelm people with React patterns at this point.
| children1: JSX.Element; // bad, doesnt account for arrays | ||
| children2: JSX.Element | JSX.Element[]; // meh, doesn't accept strings | ||
| children3: React.ReactChildren; // despite the name, not at all an appropriate type; it is a utility | ||
| children4: React.ReactChild[]; // better, accepts array children |
| @@ -59,12 +59,8 @@ Relevant for components that accept other React components as props. | |||
|
|
|||
| ```tsx | |||
| export declare interface AppProps { | |||
| children1: JSX.Element; // bad, doesnt account for arrays | |||
| children2: JSX.Element | JSX.Element[]; // meh, doesn't accept strings | |||
| children3: React.ReactChildren; // despite the name, not at all an appropriate type; it is a utility | |||
| @@ -59,12 +59,8 @@ Relevant for components that accept other React components as props. | |||
|
|
|||
| ```tsx | |||
| export declare interface AppProps { | |||
| children1: JSX.Element; // bad, doesnt account for arrays | |||
There was a problem hiding this comment.
Doesn't necessarily mean it's bad. This might be intended.
| @@ -68,7 +68,6 @@ function CommentList({ data }: WithDataProps<typeof comments>) { | |||
| } | |||
| interface BlogPostProps extends WithDataProps<string> { | |||
| id: number; | |||
| // children: ReactNode; | |||
There was a problem hiding this comment.
Unclear why this was here. better remove it if it's unused.
| @@ -346,79 +346,6 @@ class List<T> extends React.PureComponent<Props<T>, State<T>> { | |||
| } | |||
| ``` | |||
|
|
|||
| ### Generic components with children | |||
|
|
|||
| `children` is usually not defined as a part of the props type. Unless `children` are explicitly defined as a part of the `props` type, an attempt to use `props.children` in JSX or in the function body will fail: | |||
There was a problem hiding this comment.
children now have to be defined explicitly so this section is obsolete.
eps1lon
force-pushed
the
06-04-Updated_with_explicit_children
branch
from
4db5b44
to
1a6f39f
Compare
orta
approved these changes
Jun 4, 2022
filiptammergard
approved these changes
Jun 4, 2022
|
https://tkdodo.eu/blog/optional-vs-undefined explains this in more detail |
4 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Also ensured that
ReactNodechildrenare always optional. They already includeundefinedso newer TS versions would reject<Component />but not<Component></Component>which is quite annoying.Part of #495