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

[WIP] Upgrade react-router from "^3.2.6" to "^6.8.1" #2135

Closed

Conversation

lindapaiste
Copy link
Collaborator

Fixes #875


My work on this is PR paused because I've gotten far enough to see that updating react-router breaks the redux-auth-wrapper package. I need to refactor out that package before continuing.

I also need to find a way to implement the legacy setRouteLeaveHook which is used by IDEView and fix some tests which are missing the router provider.


Changes:

Updates react-router/react-router-dom from version 3.2.6 to current version 6.8.1. This is up three major versions so there are a lot of changes. It's very easy to update function components but a real pain to update class components.

Setup/Configuration:

  • router-compatibility.jsx
    • Create utility ElementFromComponent for backwards compatibility with components that are created inline and therefore difficult to instantiate via JSX. Can now convert from component={userIsAuthenticated(AccountView)} to element: <ElementFromComponent component={userIsAuthenticated(AccountView)} />.
    • Create HOC withRouter similar to the one that used to be exported from the react-router package. This is needed to access params, location, and navigate in legacy class components which cannot use hooks.
  • ./routes.jsx
    • The routes variable no longer depends on the store, as individual routes can access Redux state via context.
    • Created a wrapper Main around the App in order to handle legacy onChangeRoute and onEnter function. Renders the child content using new <Outlet/> component.
    • Routes now use element instead of component.
    • I had to convert from <Route/> elements to route objects. It should have been possible to keep the <Route/> syntax and convert it using the createRoutesFromElements utility, however this did not work due to the hot reloader package that we are using. Relevant discussion See also Improper setup of obsolete package react-hot-loader #2134
  • ./router.js
    • Create the router object with createBrowserRouter.
    • I wanted this is in a separate file in order to export the navigate function with no dependencies on other files, but it depends on the ./routes so...not the best.

Components:

  • Change import paths for Link from react-router to react-router-dom.
  • Use the hooks useParams, useNavigate, and useLocation in function components.
    • Can remove some properties from props and propTypes.
  • Replace all browserHistory.push() calls with navigate().
  • Deleted two navigations methods from DashboardView which are not actually used.

Etc:

  • In redux actions, access the navigate function as a property of the global router object.
  • Refactor the createRedirectWithUsername function.

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

…ct-router

# Conflicts:
#	client/components/PreviewNav.jsx
#	client/modules/IDE/components/ErrorModal.jsx
#	client/modules/IDE/components/QuickAddList/QuickAddList.jsx
#	client/modules/IDE/components/UploadFileModal.jsx
#	client/modules/IDE/pages/IDEView.jsx
#	client/modules/User/components/DashboardTabSwitcher.jsx
#	client/modules/User/pages/AccountView.jsx
#	client/modules/User/pages/DashboardView.jsx
@raclim
Copy link
Collaborator

raclim commented Jul 12, 2023

Thank you so much for all of your work on this so far! In the spirit of organizing some PRs, unfortunately I think I'm going to close this one for now due to the immensity of the changes, and also that I'd usually prefer to merge these types of updates incrementally!

@raclim raclim closed this Jul 12, 2023
@lindapaiste
Copy link
Collaborator Author

@raclim It's going to be hard to break this up into incremental changes because we cannot use v6 APIs without upgrading the package, and we cannot upgrade the package version without using the correct APIs. The only way to make it more incremental would be to upgrade one version at a time (3 to 4, 4 to 5, then 5 to 6).

@raclim
Copy link
Collaborator

raclim commented Jul 14, 2023

@lindapaiste Yes I was thinking along those lines, or some other way to break this down into a smaller set of changes, even if it's just two larger updates. Sorry I wasn't specific about what I meant by it! Please feel free to rework this PR and reopen it, or use it to create new ones if you'd still like to tackle this!

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