Skip to content
This repository was archived by the owner on Nov 9, 2024. It is now read-only.

Conversation

@martensm
Copy link

I believe #18 was merged too hastily. React.ReactNode isn't the correct type to use here because it disagrees with how propTypes are defined. Using React.ReactNode allows for invalid code such as this:

<>
  <Tippy content={null}>
    <div />
  </Tippy>
  <Tippy content="foo">
    {null}
  </Tippy>
</>

The second Tippy element here will fail at runtime.

@atomiks
Copy link
Owner

atomiks commented Nov 15, 2018

Thanks! Relying on the the community for this one because I'm not experienced with TypeScript to understand this myself

@atomiks atomiks merged commit e940be5 into atomiks:master Nov 15, 2018
@martensm martensm deleted the patch-typings branch November 15, 2018 19:37
@martensm
Copy link
Author

martensm commented Nov 15, 2018

To clarify, the ReactNode type definition is a union type which has null and that's why null would be allowed in TypeScript if it was typed as ReactNode. If the propTypes were PropTypes.node instead of PropTypes.element then they would be equivalent but that's not the case here.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants