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

Upgrade react-router from v3 to v4 #2294

Merged
merged 7 commits into from
Aug 2, 2023

Conversation

lindapaiste
Copy link
Collaborator

Related issue #857

This is step 1 of a multi-phase process to get up to the current v6.

I will explain what changed in react-router from v3 to v4 and what we needed to change in our codebase in order to make it work. It requires a lot of changes, as per the react-router devs: "4.0 is a complete rewrite, so you can't simply upgrade your application and change out a few things. You'll need to rethink some of how you use react-router." I roughly followed this guide.

browserHistory

There is no more browserHistory object exported from react-router. We have to create our own instance of a browserHistory using the createBrowserHistory function from the history package. I'm doing this in a new file client/browserHistory.js

Anywhere in the code where we use browserHistory must use the same global instance (note: it is a bad practice to access the history through a global variable rather than through the react-router context, but that's what we do and I haven't changed it). Imports have been changed from:

import { browserHistory } from 'react-router';

to:

import browserHistory from '../../../browserHistory';

using whatever the relative path is for that file.

Link imports

v4 introduces a separate package react-router-dom which contains the Link component and other DOM bindings. All imports have been changed from:

import { Link } from 'react-router';

to:

import { Link } from 'react-router-dom';

IndexRoute

There is no more IndexRoute component. Instead we use a regular Route and use the exact prop alongside path="/" to match the index page only.

Switch component & route matching

All of our <Route> components go inside of a <Switch> component which renders the first matching <Route> among its children. To support this "first matching <Route>" behavior, I rearranged some of the routes to place the more specific URLs first. That is, "/:username/collections/:collection_id" before "/:username/collections". Note: we could also use the exact prop on the routes if we wanted to keep the current order.

routes prop

The <Router> no longer takes a routes prop. Instead, everything is done via children. (Note: once we get up to v6 it's back to routes, kind of)

Dependency on store

There is no reason for any of the routing to require the Redux store variable as an argument. These routes are rendered inside of a redux provider and can access dispatch with the useDispatch hook, like any other component. I changed the routes variable from a function of store to just a constant.

Nested routes

There is no more nested routing in v4 (it comes back in a future version). In order to wrap all of our routes in the App container I've created a function component Routing in the client/routes.jsx file which renders the existing routes as children of App. I am keeping the const routes = separate to minimize changes. The App component needs the withRouter HOC as it is no longer a Route child.

onEnter

There is no more onEnter prop on the Route. What we had before wasn't even correct because we were calling the function instead of passing the function: onEnter={checkAuth(store)}. We want to run the auth check whenever the app is mounted so I am doing this with a useEffect hook in the Routing wrapper component. (Note: it probably makes more sense to move this into App)

onRouteChange

There is no more onChange prop on the Route. I am deleting the existing onRouteChange logic because I don't think that we need it. The specific bug which it fixes is no longer an issue.

setRouteLeaveHook

There is no more setRouteLeaveHook function on the router instance. We were using that function to pop up the "are you sure?" modal when a user clicks an internal link that navigates them away from the IDE. (Note: other triggers of that modal are handled differently). We can implement this logic by using the Prompt component. I'm doing this in a new WarnIfUnsavedChanges function component which is rendered as a child of the IDEView rather than adding the code to the IDEView component directly.

params prop

Components which are rendered by routes no longer get the router params as a top-level prop. They would have to access it from props.match.params. It would be annoying to change a bunch of _View components and their propTypes declarations. So instead I created a monkey-patched version of Route which is more backwards-compatible because it passes down the params as a top-level prop.

The Nav component is not a Route child so it's a little more annoying to fix. Instead I refactored out the usage of this.props.params because we can get the username by looking at the project (comes from Redux state) rather than the URL path.

history version

v4 of react-router requires v4 of history. I changed the redux-auth-wrapper imports in the client/utils/auth.js file to use the history4 versions instead of the history3 versions.

Testing

All <Link> components must be rendered inside of a react-router context and it is a fatal error if they aren't. This means that we need a Router provider for all of our component unit tests. I added this to the test-utils.js file. I also regenerated some test snapshots as they now include an href in the link.

I have verified that this pull request:

  • has no linting errors (npm run lint)
  • has no test errors (npm run test)
  • is from a uniquely-named feature branch and is up to date with the develop branch.
  • is descriptively named and links to an issue number, i.e. Fixes #123

@lindapaiste lindapaiste requested a review from raclim July 14, 2023 19:29
Copy link
Collaborator

@raclim raclim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great to me so far! Thanks so much for your thorough explanation, it was really helpful for referencing everything quickly!

@raclim raclim merged commit baec197 into processing:develop Aug 2, 2023
1 check passed
@lindapaiste lindapaiste deleted the chore/react-router-v4 branch August 8, 2023 02:00
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

Successfully merging this pull request may close these issues.

2 participants