FullstackAcademy / boilermaker

Code scaffold for projects
https://www.youtube.com/watch?v=7bLSuTHH4Ag&list=PLx0iOsdUOUmn7D5XL4mRUftn8hvAJGs8H
MIT License
215 stars 706 forks source link

Don't nest `Switch` components #107

Open glebec opened 6 years ago

glebec commented 6 years ago

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 commented 6 years ago

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:

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 commented 6 years ago

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.