Refactor renderRoutes to use matchRoutes

[fabianishere]

Hi again,

In relation to #6575 and as suggested in #5939, I would like to update renderRoutes to use matchRoutes instead of the Switch component.

This pull request contains an implementation of such an approach and also updates renderRoutes to render the whole tree (instead of only the first level), which removes the need for renderRoutes calls in child routes.

I have left the tests as it is for now, until it clear what the behavior of renderRoutes should be.

• Tests
• Documentation

[burnmaniac]

I don't think this solves #5429. Later paths are still not being checked because matchRoutes will not match them.

For #5429, I have a problem with different layouts on multiple routes, so my route config looks something like this:

{
    path: '/',
    component: MainAppLayout,
    routes: [
        {
            path: '/route-2',
            isExact: true,
            component: ComponentOne,
        },
    ],
},
{
    path: '/',
    component: FullScreenAppLayout,
    routes: [
        {
            path: '/route-2',
            isExact: true,
            component: ComponentTwo,
        },
    ],
},

If you go to /route-2, matchRoutes will match first / and render MainAppLayout without going to the next /, so it will never check if next / is a match and check it's child routes to see if there's a match.

I think we need something like this in order for this to work:

const matchRoutes = (routes, pathname, branches = []) => {
    routes
        .filter(route => !!route.path)
        .forEach(route => {
            // Check if the route is a match
            const match = matchPath(pathname, route);

            // If it is, check if it has child routes
            if (match) {
                if (route.routes) {
                    // If it has child routes, recursion
                    matchRoutes(route.routes, pathname, branches);
                }

                // Add the matched route to the branches
                branches.push({route: route, match: match});
            }
        });

    return branches;
};

Now, it will check the first route, if it's a match (and it is because it's /), then it will check it's child routes. After checking child routes and not finding the match, it will add / into branches and go to the next route, which is also /.
Then, it will check it's child routes, it will find /route-2, add it to the branches and also add another / to the branches.

The code is not perfect, we have a problem with duplicate routes added to the branches, but that's easy to solve.

Also, the problem is that we don't know which route is more specific and I solved it by checking location.pathname first, trying to get an exact match (always), by sorting routes.

Something like:

const matchedRoutes = matchRoutes(routes, pathname);

if (!matchedRoutes.length) {
    return null;
}

// Sort array so the most specific (exact) route is first
matchedRoutes.sort((a, b) => {
    if (b.route.path === pathname) {
        return 1;
    }

    return -1;
});

I could add this logic, but I would like to get some feedback on this, I'm not sure I'm going in the right direction.

Thanks!

[fabianishere]

@burnmaniac #6575 should solve the issue of of different layouts on multiple routes, by only matching the first exact route. If it is merged, this option can be easily carried over to renderRoutes(), now that is uses matchRoutes().

[burnmaniac]

Yes, thank you for explanation. Got kinda confused with all these pull requests. 😄

If this one is merged and #6575 is merged, then we need another one to add this option to renderRoutes.

Are there any drawbacks of this? We're not going to break something else in the router because of this? It will still work for use-cases we have right now?

[fabianishere]

If #6575 is not merged before this pull request, then we need to make another pull request to include this functionality. Otherwise, I will update it with the pull request to include it.

With regard to breaking changes, the behavior of renderRoutes is slightly changed as it doesn’t use Switch under the hood anymore and will render all levels of the matched route instead of needing to do this manually for each route. This is however also something we could put behind a flag.

[burnmaniac]

Okay, ping me if I can help somehow.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.