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

Using Fragments inside Switch #5785

Closed
bensampaio opened this Issue Dec 12, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@bensampaio

bensampaio commented Dec 12, 2017

React 16.2 added the concept of Fragments. I would like to be able to use Fragments inside Switch components to do things like this:

<Switch>
  <>
    <Route path="/" />
    <Route path="/about" />
  </>
  <>
    <Route path="/messages" />
    <Route path="/messages/new" />
  </>
</Switch>

The application I am working on is composed by small modules and all those small modules are composed together in a single app based on some conditions. Since routes need to be direct children of Switch those small modules need to expose routes as an array. However using fragments would make it a lot simpler and more readable.

@timdorr

This comment has been minimized.

Collaborator

timdorr commented Dec 12, 2017

Switch only works with the first level of components directly under it. We can't traverse the entire tree.

@timdorr timdorr closed this Dec 12, 2017

@jamesplease

This comment has been minimized.

jamesplease commented Jan 12, 2018

I could see a case being made of not traversing the entire tree (which is clearly not going to work), but instead simply checking if a particular child is a Fragment. And, if it is, checking its immediate children. This helps with cases such as:

<Switch>
  {blah.map(b => (
    <>
      <routeA/>
      <routeB/>
      <routeC/>
    </>
  ))}
</Switch>

which seem like they will become pretty common as React 16 adoption grows. The alternative, using an array syntax with keys, is pretty ugly.

@timdorr, would you consider just looking inside of Fragments, or nah?

@bripkens

This comment has been minimized.

Contributor

bripkens commented Jan 22, 2018

I am facing a similar use case for which @jmeas's proposal would be incredibly helpful (actually it is what I tried for the very reason that key usage if "pretty ugly"). Specifically, I have a set of routes that are only supposed to be enabled when a certain feature flag is enabled:

<Switch>
  <Route …/>
  <Route …/>

  {featureEnabled &&
    <>
      <Route …/>
      <Route …/>
      <Route …/>
    </>
  }
</Switch>
@bripkens

This comment has been minimized.

Contributor

bripkens commented Jan 22, 2018

As a (possibly temporary) workaround, we are now doing the following:

import { Switch } from 'react-router-dom';
import React, { Fragment } from 'react';

// react-router doesn't have support for React fragments in <Switch />. This component
// wraps react-router's <Switch /> so that it gets fragment support.
//
// https://github.com/ReactTraining/react-router/issues/5785
export default function FragmentSupportingSwitch({children}) {
  const flattenedChildren = [];
  flatten(flattenedChildren, children);
  return React.createElement.apply(React, [Switch, null].concat(flattenedChildren));
}

function flatten(target, children) {
  React.Children.forEach(children, child => {
    if (React.isValidElement(child)) {
      if (child.type === Fragment) {
        flatten(target, child.props.children);
      } else {
        target.push(child);
      }
    }
  });
}
@timdorr

This comment has been minimized.

Collaborator

timdorr commented Jan 22, 2018

@timdorr, would you consider just looking inside of Fragments, or nah?

I think that would be a good idea. But they're relatively new, so we'd need to be 100% sure they work with React <= 16.1.

@bripkens

This comment has been minimized.

Contributor

bripkens commented Jan 22, 2018

For React <= 16.1, we could check for existence of React.Fragment. If it doesn't exist, we can avoid flattening the children, right @timdorr?

If so, I would open a PR with my modified workaround and React <= 16.1 support.

@jamesplease

This comment has been minimized.

jamesplease commented Jan 22, 2018

@bripkens that should work. PR it up 🙂

@timdorr

This comment has been minimized.

Collaborator

timdorr commented Jan 22, 2018

Actually, I specifically don't want to flatten sub-children. It should only check child.type === Fragment and then return child.props.children for processing in the loop. No recursion.

@bripkens

This comment has been minimized.

Contributor

bripkens commented Jan 22, 2018

With this you mean that Switch should only analyse one level of Fragment nesting? See the PR which I just opened which has a test case for this scenario:

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

@FredyC

This comment has been minimized.

FredyC commented Jan 30, 2018

@bripkens Do you have some use case in mind where considering nested fragments might be useful? The referenced test case is not really giving any hints on that. Perhaps a complexity to support nested fragments is unnecessary?

@bripkens

This comment has been minimized.

Contributor

bripkens commented Jan 31, 2018

Here is an example for nested fragments:

// Shop.js
import productRoutes from 'products/routes';
import shoppingCartRoutes from 'shoppingCart/routes';

<Switch>
  {productRoutes}
  {shoppingCartRoutes}
</Switch>


// products/routes.js
import reviewRoutes from 'products/reviews/routes';

export default (
  <>
    <Route … />
    <Route … />
    
    {reviewRoutes}
  </>
);

// products/reviews/routes.js
import { isReviewSystemEnabled } from 'featureFlags';

export default isReviewSystemEnabled && (
  <>
    <Route … />
    <Route … />
  </>
);

Generally speaking, I am think of this in a similar way to express.js Routers. Routers can also be nested freely to get the desired route hierarchy and modularity. Especially the later is important to me. We have a large React application and defining all the routes in one file is not desirable let alone feasible (a plugin system, which is why we very much appreciated the react-router redesign). Also, having to think about what level of fragment nesting we are one, breaks modularity for us.

@FredyC

This comment has been minimized.

FredyC commented Jan 31, 2018

To be honest, the Switch itself is fairly simple component under the hood. It's not that hard just to grab it, modify it and have your own logic directly in the application. Perhaps it could be even a separate package within the repo, eg. react-router-nested-switch?

Perhaps there can be some middle ground to support customizing Switch behavior for matching. I am not able to give it some deep thinking now, but <Switch match={injectOwnLogicForMatching}> sounds reasonable enough to me.

Anyway, in my opinion it does not make sense to support fragments only partially, it could only lead to confusion unless well documented. So if there is a general disapproval from maintainers, I would rather choose one of those paths I've mentioned.

@FredyC

This comment has been minimized.

FredyC commented Jan 31, 2018

There is also another scenario that wasn't mentioned here. To me it feels more natural to wrap routes into another component. It also gives me more control as I don't need to rely on global-ish variables, I can pass it with props to have it changed during runtime.

export const LoginRoutes = ({ features }) => (
  <>
    <PasswordLoginRoute />
    {features.socialLogin ? <SocialLoginRoute /> : null}
  <>
)

export const App = ({ features }) => (
  <Switch>
    <LoginRoutes features={features} /> // notice component instead of `{loginRoutes}`
    <NotFoundRoute />
  </Switch>
)

This wouldn't be working with current PR for fragments, because the child LoginRoutes is not a fragment, but regular element. It would have to end up with traversing whole tree as @timdorr mentioned before and that's certainly wrong way to go.

My point here is, that with fragment support, it could lead to a confusion that wrapping into another component is fine. It's probably another reason why Switch should be kept as is and keep customization of it separated.

@bripkens

This comment has been minimized.

Contributor

bripkens commented Jan 31, 2018

Actually these are two very different things @FredyC. Switch cannot reasonably evaluate child component's render functions. Considering that react-router is strongly based on child property evaluation, I would see these are two distinctly different items. All fragment information is immediately available, while the result of child render functions is a lot more involved.

@FredyC

This comment has been minimized.

FredyC commented Jan 31, 2018

Yea, I kinda realized that later and made a new issue #5918 for discussion about that.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 5, 2018

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