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

Allow children to be a ReactNode instead of ReactElement #88

Merged
merged 3 commits into from May 28, 2020

Conversation

jeffvandyke
Copy link
Contributor

Since children is just returned by this component, there's no practical reason
that children has to be a single element. We've used multiple children in our
project, and this type definition made an upgrade incompatible.

Since children is just returned by this component, there's no practical reason
that children has to be a single element. We've used multiple children in our
project, and this type definition made an upgrade incompatible.
@kanbanbot kanbanbot added the WIP label May 27, 2020
@BrianMitchL
Copy link
Contributor

With v4, we switched to rendering a function component in order to use hooks. Unlike a class Component, a function component needs to return a JSX Element. To switch to a ReactNode, we'd need to wrap the render return with an element or a React Fragment. There are more details here: DefinitelyTyped/DefinitelyTyped#18051

While I would rather not break the children type, I don't necessarily like wrapping what a consumer passed to children. I would rather let the consumer control exactly what is rendered by wrapping their children in whatever markup or Fragment they'd like.

After looking into it more, it seems that the difference in types is solely a TypeScript thing, but there for historical reasons, so I'm not sure what's best. There's a good explanation here: https://stackoverflow.com/a/59840095/3928053

If you come up with a solution that passes type checking and the tests pass without changing them, I'd be happy to merge.

@jeffvandyke
Copy link
Contributor Author

Ahh, that makes sense :) I'll do some more investigating...

@jeffvandyke
Copy link
Contributor Author

Long story short - based on Flow's docs and React's behavior, DefinitelyTyped's typings for React.FunctionComponent's return (ReactElement | null) seem incorrect (probably should be ReactNode). I'll prepare and submit a PR to them, hold this PR on that, I'll post updates here.

@BrianMitchL
Copy link
Contributor

There's been an issue open about that for a few years DefinitelyTyped/DefinitelyTyped#18051

@jeffvandyke
Copy link
Contributor Author

OK, that was more complicated than I thought, so I've updated this PR to use the casting children as ReactElement workaround, seems better than introducing another Fragment layer to the component tree, and it's a workaround that's been done before.

Also updated README.md like I should have before.

Copy link
Contributor

@BrianMitchL BrianMitchL left a comment

Choose a reason for hiding this comment

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

Nice investigation! Casting seems like a good solution to this.

@BrianMitchL BrianMitchL merged commit 76a0fba into buildo:master May 28, 2020
@kanbanbot
Copy link

@giogonzo
Copy link
Member

giogonzo commented Jun 1, 2020

released as v4.0.2

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.

None yet

4 participants