Skip to content

Remove single child restriction for <Router> #5712

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

Closed
wants to merge 3 commits into from

Conversation

pshrmn
Copy link
Contributor

@pshrmn pshrmn commented Nov 9, 2017

This PR takes advantage of the fact that with React 16, a component can return multiple elements.

// 15
ReactDOM.render((
  <BrowserRouter>
    <div>
      <One />
      <Two />
    </div>
  </BrowserRouter>
), holder);

// 16
ReactDOM.render((
  <BrowserRouter>
    <One />
    <Two />
  </BrowserRouter>
), holder);

The invariant call will still throw in React 15 if the user passes multiple children to a <Router>. I'm not actually sure if there is a convenient way to test this. If someone is using React 15 and attempts to pass multiple children, the invariant will throw before the render function is called, so the changes to the render function should have no effect

@pshrmn pshrmn added the v5 label Nov 9, 2017
@timdorr
Copy link
Member

timdorr commented Nov 9, 2017

Is this possible at the top level? Didn't know that!

I'm not sure how I feel about a version check. Feels like browser sniffing, which is bad. It should at least be isolated to utility module so we can re-use it elsewhere and test it.

@pshrmn
Copy link
Contributor Author

pshrmn commented Nov 9, 2017

Yeah, I hadn't even thought about this until I was reminded of the "aux component" earlier

const Aux = props => props.children

I didn't want to do a version check, but I wasn't really sure how to continue supporting v15 otherwise.

I'm not actually sure if this is necessary. I preferred grouping the actual app components in a single <App>, but it would still be nice to get rid of the wrapper element.

@timdorr timdorr added this to the 5.0 milestone Nov 10, 2017
@timdorr timdorr force-pushed the master branch 2 times, most recently from fb3a586 to f9481bb Compare January 26, 2018 13:04
@pshrmn
Copy link
Contributor Author

pshrmn commented Mar 14, 2018

I updated this to throw if multiple children are provided and React.Fragment does not exist. This isn't perfect since React.Fragment was added in 16.2; maybe this PR should just wait until RR bumps its React peer dep to >=16?

@timdorr timdorr force-pushed the master branch 4 times, most recently from 8f33936 to 1fb8696 Compare April 26, 2018 20:30
@mjackson mjackson removed the v5 label Sep 12, 2018
@mjackson mjackson closed this in baef8ab Sep 21, 2018
@pshrmn pshrmn deleted the many-children branch September 21, 2018 02:09
@lock lock bot locked as resolved and limited conversation to collaborators Nov 20, 2018
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.

3 participants