Upgrade to React Router v6

jroebu14 commented 2 years ago

Is your feature request related to a problem? Please describe. We want to upgrade the react-router package from v5 to v6 so that the dependency is up to date and more likely to receive security patches and new features. We also want to take advantage of the bundle size savings from upgrading this package.

Describe the solution you'd like We would like to upgrade to React Router v6. After some investigation, it seems to require quite a bit of refactoring around our implementation of React Router.

Here is a guide to upgrading from v5.

I'll try to outline what I think needs to be done in Simorgh to support v6. Please note that there may be better approaches than what I've suggested so feel free to suggest alternative approaches in the comments.

Summary of work required

Packages
Render the Route components in App.jsx
Remove regex use from Route paths
Various migrations to new APIs

You can see a messy and unfinished example of these changes in this PR.

Packages

Upgrade react-router and react-router-dom
Remove react-router-config. All of the functionality from v5's react-router-config package has moved into core in v6 and is now a hook named useRoutes. However, useRoutes does not have a way of passing extra props to Route components that we depended on for passing pageData and other props so we cannot make use of it anymore. You'll see a different approach further down that was inspired by this article.
React Router maintainers suggest removing the history package because it's now a direct dependency of v6 rather than peer dependency however we make use of this package for testing. With v6 we should never use history in app code and instead should use useNavigate() for navigations.

Render the Route components in `App.jsx`

As mentioned earlier, we need to remove react-router-config and use of the renderRoutes method. v6 comes with a similar method, useRoutes but the API is slightly different in that it doesn't allow us to pass in extra props (the 2nd argument of renderRoutes) we can send to the rendered Route component as shown in this example:

// from App.jsx - react-router-config use
return renderRoutes(routes, {
    ...state,
    bbcOrigin,
    previousPath: previousPath.current,
    loading: routeHasChanged,
    showAdsBasedOnLocation,
  });

In order to pass extra props to the rendered Route component we can instead render our routes like this:

  const renderRoute = route => {
    const { component: Component } = route;

    return (
      <Route
        key={route.path}
        path={route.path}
        element={
          <Component
            {...state}
            bbcOrigin={bbcOrigin}
            previousPath={previousPath.current}
            loading={routeHasChanged}
            showAdsBasedOnLocation={showAdsBasedOnLocation}
          />
        }
      />
    );
  };

  return (
    <Routes>
      {routes.map(renderRoute)}
    </Routes>
  );

In this example we just map the routes object into jsx and pass the extra props to the components. This approach was inspired by this article.

Remove regex pattern matching from Route paths

v6 removed path-to-regexp and now only supports dynamic :id-style params and * wildcards.

Simorgh path pattern matching depends on path-to-regexp's more advanced syntax so we need to rethink how we write our path patterns to match to the correct routes.

For example, this is how we'd match all service's home pages in v5:

// src/app/routes/home/index.js - v5

export default {
  path: `/:service(${serviceRegex}):variant(${variantRegex})?:amp(${ampRegex})?`,
  exact: true,
  component: FrontPage,
  getInitialData,
  pageType: FRONT_PAGE,
};

Without regex pattern matching we will need to map over all services and create route objects, do the same for amp and then explicitly include the service variants. An example of this might look like:

// src/app/routes/home/index.js - v6

const CANONICAL_PATHS = allServices.map(service => `/${service}`);
const CANONICAL_SERVICE_VARIANT_PATHS = [
  '/zhongwen/simp',
  '/zhongwen/trad',
  '/serbian/cyr',
  '/serbian/lat',
];

const AMP_PATHS = CANONICAL_PATHS.map(path => `${path}.amp`);
const AMP_SERVICE_VARIANT_PATH = CANONICAL_SERVICE_VARIANT_PATHS.map(
  path => `${path}.amp`,
);

const component = FrontPage;
const pageType = FRONT_PAGE;

const frontPageRoutes = [
  CANONICAL_PATHS,
  CANONICAL_SERVICE_VARIANT_PATHS,
  AMP_PATHS,
  AMP_SERVICE_VARIANT_PATH,
]
  .flat()
  .map(path => ({
    path,
    component,
    getInitialData,
    pageType,
  }));

Similarly for article pages we would do:

// src/app/routes/article/index.js - v5

path: `^/(${serviceRegex})(${articleLocalRegex})/(${idRegex})(${variantRegex})?(.amp)?$`,

// src/app/routes/article/index.js - v6

const CANONICAL_PATHS = allServices.map(service => `/${service}/articles/:id`);
const CANONICAL_SERVICE_VARIANT_PATHS = [
  '/zhongwen/simp/articles/:id',
  '/zhongwen/trad/articles/:id',
  '/serbian/cyr/articles/:id',
  '/serbian/lat/articles/:id',
];
const CANONICAL_UK_PATHS = [
  '/cymrufyw/erthyglau/:id',
  '/scotland/sgeulachdan/:id',
];

const AMP_PATHS = CANONICAL_PATHS.map(path => `${path}.amp`);
const AMP_SERVICE_VARIANT_PATH = CANONICAL_SERVICE_VARIANT_PATHS.map(
  path => `${path}.amp`,
);
const AMP_UK_PATHS = CANONICAL_UK_PATHS.map(path => `${path}.amp`);

This has one drawback though - because we are not using named parameters e.g. :service we don't get these as params props e.g service or variant so any use of useParams or useLocation won't have service or variant in the result.

Various migrations to new APIs

There's various migration changes to v6 APIs that we need to make but the upgrade guide will explain these better.

From what I can see the things that affect us include:

import { StaticRouter } from 'react-router-dom'; -> import { StaticRouter } from 'react-router-dom/server';
useRouteMatch -> useMatch
Redirect to Navigate
Route component's component -> element

There may be more I have missed though.

Describe alternatives you've considered

Instead of upgrading to v6 all at once, we wait for the backwards compatibility package that brings all of the v5 functionality to v6 enabling us to incrementally upgrade to v6 fully.
We stay on v5 until v6 has support for regex paths. It's hinted in the migration guide - "please know that we plan to add some more advanced param validation in v6 at some point". There is no indication when this will be or if it will have all of the features that v5 did.
This one is a bit out there but we might want to consider looking at a React framework that abstracts routing (and other low-level things) for us so that we don't need to invest as much time in this kind of work.

Testing notes [Tester to complete]

Dev insight: Will Cypress tests be required or are unit tests sufficient? Will there be any potential regression? etc

[ ] This feature is expected to need manual testing.

Checklist

[ ] (BBC contributors only) This issue follows the repository use guidelines

Additional context Related to https://github.com/bbc/simorgh/pull/9645

andrewscfc commented 2 years ago

Thanks so much for putting together this detailed issue @jroebu14, I've read through it and taken a look at the migration guide myself as well, here's what I've taken away personally:

The changes aren't hugely advantegeous to us, maybe some bundle size improvements?
- Would it be easy to quantify these improvements, it may justify doing the work moreso?
A backwards compatibility package is on the way

As with all package upgrades, our primary motivation is to stay up-to-date so we don't have an insecure dependency we can't upgrade. Second to that is staying up-to-date with the latest approach so our codebase stays modern and is not surprising to future maintainers.

Imo, I'd wait for the backwards compatibility package to come along and get ourselves upgraded and secure when that comes along.

Upgrading the V6 properly is more debatable in terms of timing, it looks non-trivial especially with the loss of regex routes; the change would need quite a lot of thought and quite lot of testing. I wonder if we make the change when we roll-out clientside navigation more widely. @aeroplanejane where do we see clientside navigation in our product roadmap? Maybe when we roll-out optimo articles more extensively? We could opportunistically refactor this area when we take that work on, we would be testing clientside navigation anyway as part of the work.

aeroplanejane commented 2 years ago

Thanks for the write-up @jroebu14 @andrewscfc Agree this should be parked for the time being given there is no immediate urgency and it's quite a big effort.

Clientside navigation is not currently roadmapped for this quarter. If / when we do rollout it out we would have to look at certain page journeys given the issue with includes. I've linked this issue to NEWSWORLDSERVICE-1411 so we don't lose it.

Is there a point at which this will have to be tackled from a security / resiliency perspecitive? cc @gavinspence @joshcoventry

andrewscfc commented 2 years ago

Assuming the backwards compatability package becomes available we will be at no risk for a security perspective, we will just be using an increasingly outdated API harming our maintainability. I personally would only upgrade to the new API when/if we do clientside navigation.

joshcoventry commented 2 years ago

Closing for now, we can revisit when we do clientside navigation.

joshcoventry commented 2 years ago

Re-opening as it still warrants further discussion

jroebu14 commented 2 years ago

Was chatting Josh there - there is bundle size improvements with this work. I think the most meaningful aspect of this work is the chance to improve routing in Simorgh. This is the main reason I looked into this work. It would be a chunky refactor but I think a worthwhile one.

bbc / simorgh