Skip to content

fix: allow render function to return null - #669

Merged
developit merged 3 commits into
preactjs:masterfrom
KnisterPeter:typings
Jul 2, 2017
Merged

fix: allow render function to return null#669
developit merged 3 commits into
preactjs:masterfrom
KnisterPeter:typings

Conversation

@KnisterPeter

Copy link
Copy Markdown
Contributor

In react components are allowed to return null from the render function

In react components are allowed to return null from the render function
@coveralls

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling b8ad2f9 on KnisterPeter:typings into dee8c00 on developit:master.

@developit

Copy link
Copy Markdown
Member

Any TS folks like @mseddon want to approve?

@mseddon

mseddon commented Apr 25, 2017

Copy link
Copy Markdown
Contributor

LGTM!

Comment thread src/preact.d.ts Outdated
@@ -1,5 +1,5 @@
declare namespace preact {
interface ComponentProps<C extends Component<any, any> | FunctionalComponent<any>> {
interface ComponentProps<C extends Component<PropsType, any> | FunctionalComponent<PropsType>, PropsType> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What's the reason for adding PropsType here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Isn't it just more specific/correct than 'any'?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As PropsType isn't used in this interface and isn't constrained in any way, it provides no more safety than simply any, and only adds complexity.

@developit developit added the types label Jul 2, 2017
@developit
developit merged commit 57341a5 into preactjs:master Jul 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants