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

Remove React.Children.only from theme provider #1325

Closed
clayne11 opened this Issue Nov 23, 2017 · 10 comments

Comments

Projects
None yet
3 participants
@clayne11
Copy link

clayne11 commented Nov 23, 2017

It there a reason to have React.Children.only in the theme provider? It seems unnecessary and it makes certain layouts not possible. Why not simply return this.props.children from render?

@mxstbr

This comment has been minimized.

Copy link
Member

mxstbr commented Nov 23, 2017

Because the <ThemeProvider /> can be at the top of the component tree, near your ReactDOM.render. Because it just returns its children when rendering, that will go straight to up to the top. So, this:

ReactDOM.render(
  <ThemeProvider>
    <div />
    <div />
  </ThemeProvider>
)

is basically this:

ReactDOM.render(
  <div />
  <div />
)

And that's not valid input for render.

So, we use React.Children.only to ensure we only have one child (That's the main use case for that function) and yell at you if you try to have more. Sorry for the troubles there, but it's for everybody's benefit!

@mxstbr mxstbr closed this Nov 23, 2017

@clayne11

This comment has been minimized.

Copy link

clayne11 commented Nov 23, 2017

I have multiple different themes throughout my app. I don't only use the theme at the top level.

Also, the example you gave isn't valid, but as of React 16, this is:

ReactDOM.render([
    <div>DIVE ONE</div>, 
    <div>DIVE TWOOOOO</div>
], anchor)

Are you sure that what I'm suggesting won't work in React 16?

There are a lot of different use cases and I don't think that styled-components should be so prescriptive of how it can be used. In my use case what I'm suggesting is perfectly valid.

@mxstbr

This comment has been minimized.

Copy link
Member

mxstbr commented Nov 24, 2017

You are right, my wording was bad, I've updated my previous comment: The ThemeProvider doesn't belong to the top, but it can be at the top.

Of course React v16 supports Arrays, but we're not dropping support for React <v16 given how many people still use those versions. We might cut a major version in a year or two and drop support, but to be honest I'm not very inclined to drop support for React <v16 at all if the only difference is that the ThemeProvider directly returns the children.

That being said, I'd be happy to hear more about your specific use case, do you have some code that doesn't work solely due to this behavior?

@clayne11

This comment has been minimized.

Copy link

clayne11 commented Nov 25, 2017

I do. I want to put multiple children under a theme and I currently have to add an unnecessary wrapper div because of the React.Children.only on the ThemeProvider.

I understand that you're trying to protect some newer users of React from making a mistake but in doing so you're limiting some of the potential use cases for styled-components.

If you're really concerned we could add a allowArrayChildren flag to the ThemeProvider that wouldn't force the use of an only child but the default would stay the same.

I understand that you want to support React 15 for another while but that shouldn't hamper users of React 16 of which there are already a lot and more people are upgrading every day.

@mxstbr

This comment has been minimized.

Copy link
Member

mxstbr commented Nov 25, 2017

That's a lot more work than you just putting a <div /> there, to be honest 🤷‍♂️

@clayne11

This comment has been minimized.

Copy link

clayne11 commented Nov 25, 2017

Wrapper divs are a huge annoyance because they have to be styled depending on the context that they're being used in. It's also an unnecessary div to render which impacts performance for no reason.

The only work really required here IMO is to simply return this.props.children from render rather than forcing it to be an only child. I can't imagine there's that many people trying to render multiple children in React 15 that can't figure out how to debug this issue.

@mxstbr

This comment has been minimized.

Copy link
Member

mxstbr commented Nov 25, 2017

I'm sure a single wrapper div is going to kill your apps performance 😉

Nobody's talking about the code change, of course changing three lines of code is easy. I can do that in two minutes tops.

The problem is what comes afterwards: We gotta update all the documentation and any old blog posts, write a changelog documenting this and the reason for changing it, and once that's done we gotta deal with the dozens of questions we'll get about that weird React error when you try rendering a top-level ThemeProvider.

For what?

I don't even know how you're using the ThemeProvider, can you please provide the code from your app that is so much harder to write due to our implementation? I'd love to help you resolve that styling issue, because I'm 100% it's resolvable.

@pluma

This comment has been minimized.

Copy link

pluma commented Dec 8, 2017

Can you clarify how removing the enforcement breaks backwards compatibility? The way I see it existing users should not see any problem because they already only use a single child and for new users using React 16 it will just work as expected. New users on React 15 will just get an error that can easily be explained in the docs with a note like

If you're using React 15 or earlier you need to make sure that <ThemeProvider> is only passed a single element as child. Otherwise you may see an error like this: (actual error printed by React 15 in the console)

I would expect the existing restriction to result in at least as many questions going forward as not having that restriction would for the time being (i.e. not many). And as users upgrade to 16 and beyond, there will likely be more questions from having the restriction than not having it.

FWIW I'm not sure but I think React.Fragment could be used as a userland workaround in React 16.2.0 and later if you want to use multiple children?

@pluma

This comment has been minimized.

Copy link

pluma commented Dec 8, 2017

To clarify, here's what I'm seeing in React 15 without the restriction:

MyProviderThingy(...): A valid React element (or null) must be returned. You may have returned undefined, an array or some other invalid object.

Demo: https://codesandbox.io/s/54lr0o8yn

And here's what I'm seeing with the restriction:

React.Children.only expected to receive a single React element child.

Demo: https://codesandbox.io/s/zx4k75y304

I don't think either message is more helpful than the other, but the one without the restriction gives the name of the component that returned multiple children whereas having the restriction hides that from the error message but gives a slightly more relevant (but not really helpful) stacktrace.

@clayne11

This comment has been minimized.

Copy link

clayne11 commented Dec 8, 2017

I couldn't agree more @pluma.

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