Repository navigation
Add typescript child type checking - #1116
Conversation
There was a problem hiding this comment.
Is this correct? I was under the impression that the children prop is always an array.
There was a problem hiding this comment.
Not sure now it's been pointed out... it didn't want to type check correctly without changing children in some form though.
I ended up doing it a similar way to the DT react definitions, if there's a type difference between them there may be a more accurate way of doing it for preact but I'm not sure what it would be
There was a problem hiding this comment.
Yep, this is correct. The children prop is always an array at runtime, but for jsx type inference in TS they are only of type array when there are more than one child. Example:
type RenderFn = (type: string) => void;
class Foo extends Component<{children: Array<string | RenderFn>}> {
render() {
return <div>{this.props.children}</div>;
}
}
function Bar() {
// Type checked as NON-array, because the component has one child.
return <Foo>{(type) => undefined}</Foo>;
}
function Baz() {
// Type checked as an Array, because the component has now 2 children
return <Foo>2{(type) => undefined}</Foo>;
}There was a problem hiding this comment.
@Alexendoo can you change {} to object? That way we'd be consistent with the rest of our typings.
There was a problem hiding this comment.
@marvinhagemeister why the use of object here? Taking a look on ReactNode it has the following types:
type ReactNode =
| ReactElement
| string
| number
| Iterable<ReactNode>
| ReactPortal
| boolean
| null
| undefined
| DO_NOT_USE_OR_YOU_WILL_BE_FIRED_EXPERIMENTAL_REACT_NODES[
keyof DO_NOT_USE_OR_YOU_WILL_BE_FIRED_EXPERIMENTAL_REACT_NODES
];There was a problem hiding this comment.
Perhaps we may refactor it like:
type ComponentChild = VNode<any> | string | number | Iterable<ComponentChild> | bigint | boolean | null | undefinedThere was a problem hiding this comment.
At the time React had an object in its definition also but I do not remember why (this is a 6 year old PR 😅), type definitions could also not refer to themselves like that back then
There was a problem hiding this comment.
Yeah pretty old 😅 now that is possible to refer to a type by itself; would you mind if I create a PR to update ComponentChild types? object is pretty wide and we can change it by Iterable<ComponentChild>.
There was a problem hiding this comment.
I'm not a maintainer here but I'd say go for it yeah, it'll be more visible place to have the discussion at the very least
There was a problem hiding this comment.
Just created the PR, @Alexendoo thank you so much for answering.
marvinhagemeister
left a comment
There was a problem hiding this comment.
LGTM, thanks for your contribution 🎉 @Alexendoo
Can you change {} to object? Happy to merge this and that's really awesome that you figured this out 👍 👍
| type ComponentChild = JSX.Element | string | number | null; | ||
| type ComponentChildren = ComponentChild[]; | ||
| type ComponentChild = VNode<any> | string | number | null; | ||
| type ComponentChildren = ComponentChild[] | ComponentChild | {} | string | number | null; |
There was a problem hiding this comment.
Yep, this is correct. The children prop is always an array at runtime, but for jsx type inference in TS they are only of type array when there are more than one child. Example:
type RenderFn = (type: string) => void;
class Foo extends Component<{children: Array<string | RenderFn>}> {
render() {
return <div>{this.props.children}</div>;
}
}
function Bar() {
// Type checked as NON-array, because the component has one child.
return <Foo>{(type) => undefined}</Foo>;
}
function Baz() {
// Type checked as an Array, because the component has now 2 children
return <Foo>2{(type) => undefined}</Foo>;
}| type ComponentChild = JSX.Element | string | number | null; | ||
| type ComponentChildren = ComponentChild[]; | ||
| type ComponentChild = VNode<any> | string | number | null; | ||
| type ComponentChildren = ComponentChild[] | ComponentChild | {} | string | number | null; |
There was a problem hiding this comment.
@Alexendoo can you change {} to object? That way we'd be consistent with the rest of our typings.
It also allows type inference (and checking) of function bodies. A basic example is given in the tests that infers the type of `num`
|
Sure thing, changed it to |
|
The types look wrong to me. According to #1008 (comment) the children should always be an array. The way it has been defined now looks no different than |
|
Yeah this is correct @valotas it is always an array at runtime, this definition is necessary to appease the assumptions TSX makes, it's summed up nicely here #1116 (comment) It may be possible in TypeScript 3 using LibraryManagedAttributes to do this correctly on both sides however, but I haven't explored this yet |

Closes #1086
It fixes the original issue from #1008, it will now type check correctly
It will also allow type inference, a basic example is provided in the tests that infers the type of
numbut it will be useful for anything like preactjs/preact-router#269