Skip to content
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

Add typescript child type checking #1116

Merged
merged 2 commits into from May 28, 2018

Conversation

Alexendoo
Copy link
Contributor

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 num but it will be useful for anything like preactjs/preact-router#269

src/preact.d.ts Outdated
type ComponentChild = JSX.Element | string | number | null;
type ComponentChildren = ComponentChild[];
type ComponentChild = VNode<any> | string | number | null;
type ComponentChildren = ComponentChild[] | ComponentChild | {} | string | number | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? I was under the impression that the children prop is always an array.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Alexendoo can you change {} to object? That way we'd be consistent with the rest of our typings.

Copy link
Member

@marvinhagemeister marvinhagemeister left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍 👍

src/preact.d.ts Outdated
type ComponentChild = JSX.Element | string | number | null;
type ComponentChildren = ComponentChild[];
type ComponentChild = VNode<any> | string | number | null;
type ComponentChildren = ComponentChild[] | ComponentChild | {} | string | number | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>;
}

src/preact.d.ts Outdated
type ComponentChild = JSX.Element | string | number | null;
type ComponentChildren = ComponentChild[];
type ComponentChild = VNode<any> | string | number | null;
type ComponentChildren = ComponentChild[] | ComponentChild | {} | string | number | null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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`
@Alexendoo
Copy link
Contributor Author

Sure thing, changed it to object @marvinhagemeister

@coveralls
Copy link

coveralls commented May 28, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling 82d3da7 on Alexendoo:typed-children into feb96b5 on developit:master.

@valotas
Copy link
Contributor

valotas commented Aug 17, 2018

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 any

@Alexendoo
Copy link
Contributor Author

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

@valotas
Copy link
Contributor

valotas commented Aug 19, 2018

@Alexendoo thanks a lot for the explanation! I'll have a look at ts3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Typescript JSX child type checking
5 participants