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

Don't nest Switch components #107

Open
glebec opened this issue Jun 9, 2018 · 2 comments
Open

Don't nest Switch components #107

glebec opened this issue Jun 9, 2018 · 2 comments

Comments

@glebec
Copy link
Member

glebec commented Jun 9, 2018

React-Router explicitly states that only Router and Route components should be children of Switch.

We have nested Switches in our current codebase, in order to make some routes exist only if the user is logged in. However, if a user tries to go to a non-recognized route, we currently render a fallback route until the user is asynchronously logged in (at which point one would hope the route becomes recognized).

Unfortunately, this fallback route does not trigger if placed below the nested Switch and the user is logged in, but the route does not match anything in the sub-switch. This is because Switch components are misinterpreted by the parent Switch as actually being an unconditional (pathless) route – so no routes below them will ever trigger.

A student had this issue. The solution we came up with was to duplicate the fallback route inside the nested switch, which wasn't very DRY. The student subsequently created an issue here. A user responded with the following alternative:

If it were me, I would probably write a custom <Switch> component that is better suited for this use case. It is really just a loop over props.children, so implementing your own isn't very difficult.

@collin
Copy link
Contributor

collin commented Jun 20, 2018

I was thinking maybe we could find a solution if we flattened the loggedIn routes. I'm not 100% sure I'm parsing the issue as described, so this might not be a step in the right direction anyway. Ultimately, don't think this is a good avenue to explore as a change to boilermaker. But I did find it interesting.

        <Switch>
          {someCondition && [
            <Route
              path="/conditional-a"
              component={() => <h1>Conditional A</h1>}
            />,
            <Route
              path="/conditional-b"
              component={() => <h1>Conditional B</h1>}
            />,
          ]}
          <Route
            path="/non-fragment"
            component={() => <h1>Not Conditional</h1>}
          />
          <Route
            path="/"
            component={() => <h1>Home</h1>}
          />
        </Switch>

Arguments against this:

  • You get the elements in an array should have a key warning.
  • It mixes an array that looks A LOT like JSX and JSX side by side.

I also tried using <React.Fragment> instead of the array, but React.Children.* does not interpret fragment children the way we'd want it to.

@glebec
Copy link
Member Author

glebec commented Jul 21, 2018

Hm, that's an interesting possibility. I think the key issue could obviously be mitigated by, well, adding keys (the valye would be the path. But that gets even more verbose / weird.

Maybe we can double down on this. Put all routes in arrays (and map over them to add keys?), then make the Switch contain a concatenation of them, e.g. [...publicRoutes, ...loggedInRoutes, ...fallbackRoutes] or similar.


Alternatively, we back off from truly dealing with this in Boilermaker, but put in a comment that says if the inner switch activates, nothing below it will work, so users need to add a fallback route to the inner switch if applicable.

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

No branches or pull requests

2 participants