Support fragments in <Switch />

[bripkens]

When checking for matched routes in <Switch />, traverse fragments to
determine the matched route.

Fixes #5785

[timdorr]

We specifically should not support this. We don't support nested routes in a Switch, so this behavior difference will undoubtedly cause confusion. To be clear, I see this:

<Switch>
  <Route path="/foo" component={Foo} />
  <>
    <Route path="/bar" component={Bar} />
    <Route path="/baz" component={Baz} />
  </>
</Switch>

as equivalent to this:

<Switch>
  <Route path="/foo" component={Foo} />
  {[barRoute, bazRoute].map(route => <Route path={route.path} component={route.component} />)}
</Switch>

Fragments are basically convenience functions for arrays. So, I'd treat them as such to maintain the status quo with Switch's behaviors.

[bripkens]

Actually React.Children.forEach recurses over nested arrays. Meaning: react-router always supported this very same case, i.e. arrays in arrays. For clarification, I added more test cases that use arrays instead of fragments. The behavior is only consistent when recursing over fragments.

[bripkens]

See: https://github.com/ReactTraining/react-router/pull/5892/files#diff-00d924998d3d7fd7c087362a6e4bc6bcR363

[bripkens]

Relevant React code:

https://github.com/facebook/react/blob/4ca7855ca062d5d7dfca83c86acf46731e1e57ef/packages/react/src/ReactChildren.js#L153

and docs:

https://reactjs.org/docs/react-api.html#reactchildrentoarray

[bripkens]

One might argue that this is an inconsistency within React itself. Related:

facebook/react#11859 (comment)

[timdorr]

Actually, I'm not. Sorry, just busted out some code here and contradicted myself. Apologies for the flip flop!

React.Children.count([<div/>, <div/>, [<div />, <div/>]]) == 4
React.Children.count([
  <div/>,
  <div/>,
  <React.Fragment>
    <div/>
    <div/>
  </React.Fragment>
]) == 3

Fragments are not the same as arrays, they just have a similar use case. The post here is clarifying: facebook/react#11859 (comment)

I'd much rather do React.Children.map(children, child => child.type === React.Fragment ? child.props.children : child).forEach(child => ...). It's not common to have nested child arrays inside of <Switch>, so trying to emulate that behavior with something that's intentionally inconsistent.

[bripkens]

I do not see why fragments should be handled differently for all intents and purposes of react-router than arrays. They are element containers and should be handled accordingly.

Design decisions have been made for fragments which result in different behavior for React.Children.count, React.Children.map and others when comparing fragment and array behavior. This difference in behavior stems from important differences when handling these containers as part of render. React-router doesn't have to behave different when encountering fragments vs. arrays.

Furthermore, it makes no sense for react-router to try to matchPath a fragment element. This will never work.

Lastly, there is no technical reason why fragment recursion cannot be supported. There is no additional cost associated to it. It is faster than a map followed by a forEach. It makes array/fragment behavior consistent.

[timdorr]

I do not see why fragments should be handled differently for all intents and purposes of react-router than arrays.

Because they're not arrays. I'd really rather not try to go against the upstream behavior semantics, even if it is admittedly weird. We got burned in previous versions by trying to do too much of React's behavior on our own. The edge cases are death by a thousand cuts.

I'm still on Team No Flatten™, but the rules dictate we have two maintainers approve before merging. If anyone else wants to voice their opinions, even if it's different from my own, they can and we can work towards a resolution.

[jamesplease]

I'm not a maintainer, but I recommended this change over in #5785 , so I thought I'd voice that I'm also on Team No Flatten for whatever that's worth. Thanks for all of your work @timdorr and @bripkens!

[bripkens]

Added the requested example to the issue:

#5785 (comment)

[pshrmn]

We just reference properties of the React object everywhere else (React.Component, React.Children), so we should probably just use React.Fragment. If I recall correctly, the { Fragment } style of destructured imports is actually a Babel feature (there is no actual Frament export from React).

[bripkens]

Fair point. Changed.

[timdorr]

Can we move this within the component? We don't need it externally and then we can access the props/context directly.

[bripkens]

I can move it within the component sure, but do know that this was a deliberate decision for the sake of efficiency. There is no need to redefine it over and over again within render. Especially considering that it does not need access to this, props or state.

Please let me know whether you still would like this to be inlined.

[jamesplease]

@bripkens moving it within the component doesn't necessarily mean defining it within render, right? It could go on the instance, or possibly as a static class method if React Router transpiles those down.

[bripkens]

Whoops, I thought the component was just a function when I looked at it again on GitHub. Sorry for that! Changed it accordingly.

[mjackson]

What's your use case, @bripkens? I can't really think of a compelling use case for having a <Fragment> inside of a <Switch>.

The only thing I can imagine that you might want to do is return multiple <Route>s from one of your own components, e.g. a <UserRoutes> component that returns a <Fragment> containing multiple <Route>s. But that's something that we don't currently support.

The lack of a clear use case, plus the fact that React treats arrays differently from <Fragment>s is enough for me to think this isn't a good idea.

[jamesplease]

@mjackson i listed a use case over in #5785 (comment) . If you read that issue and other similar issues, there are a handful of what I think to be other good use cases from developers.

tl;dr keys are unnecessary boilerplate with groups of routes. Fragments let you group the routes together in a way that adds no extra boilerplate. Whether that is because they are dynamically generated, or because they need to be grouped for auth permissions, or just because a developer wants to group them based on their role in the app.

plus the fact that React treats array differently from <Fragment/>

🤷‍♂️ the users of React Router are not going to care about this difference. They are just going to want to use Fragments so that they don’t have to specify keys when working with groups of routes within a single Switch.