CUMULUS-3862: Upgrade React Router to v6 #1148

jjmccoy · 2024-09-25T13:07:16Z

Summary: React Router from v5 to v6

Addresses CUMULUS-3862: Upgrade React Router from v5 to v6

Changes

Detailed list or prose of changes:
- Migrate from React Router v5 to v6 (https://reactrouter.com/en/main/upgrading/v5)
- Update routing to new v6 standard in App.js and component files
- Update corresponding unit tests

PR Checklist

Update CHANGELOG
Unit tests
Adhoc testing
Integration tests

npauzenga

@jjmccoy leaving the first part of this review. I've looked at 74/100 files and I want to get something up as it's taking a while to go through everything. Still going through the remaining 26 files and I'll post another review for those.

Also I see there are Cypress tests failing. Are those flukes or likely to require more changes?

npauzenga · 2025-02-13T19:30:16Z

.eslintrc

@@ -52,14 +52,19 @@
    "react/react-in-jsx-scope": 2,
    "react-hooks/rules-of-hooks": "error",
    "react-hooks/exhaustive-deps": "warn",
-    "eslint-comments/no-unused-disable": "warn",
+    "eslint-comments/no-unused-disable": "off",


Is this just because we don't heed the warnings?

npauzenga · 2025-02-13T19:31:06Z

CHANGELOG.md

+- **CUMULUS-3862**
+  - Updated React Router v5.2.0 to v6.26.2
+  - Updated routes and hooks for app navigation
+  - Established new router with withRouter.js HOC


What's HOC?

HOC = Higher Order Component. In React, they allow you to reuse component logic. With the upgrade to v6 the dependency 'withRouter.js' and eventually 'history.js' (when Redux ticket is complete) no longer. We need to apply the hooks 'useDispatch', 'useNavigate', etc. that React Router and Redux use now.

npauzenga · 2025-02-13T19:31:45Z

CHANGELOG.md

+  - Updated routes and hooks for app navigation
+  - Established new router with withRouter.js HOC
+  - Established withUrlHelper.js HOC to later replace URL params utility
+  - Updated unit tests to accommodate component changes


Probably don't need to call out test updates explicitly. The CL (in my opinion anyway) should just be used to communicate functional changes to users.

Oh okay. I thought since they were dramatically changed to include the React Router hooks but yeah I can remove that.

Removed. Changes committed.

npauzenga · 2025-02-13T19:32:50Z

app/src/index.js

@@ -13,5 +13,5 @@ import App from './js/App';
      axe.default(React, ReactDOM, 1000);
    });
 } */
-
-ReactDOM.createRoot(document.getElementById('site-canvas')).render(<App />);
+const root = createRoot(document.getElementById('app'));


Did we actually change the id from site-canvas to app? Will delete this comment if it becomes clear later.

Yes. Changed from 'site-canvas' to 'app' to be consistent with React documentation when referencing.

npauzenga · 2025-02-13T19:36:28Z

package.json

@@ -160,7 +160,7 @@
    "deepmerge": "^4.2.2",
    "final-form": "^4.20.4",
    "global": "^4.3.1",
-    "history": "^4.10.1",
+    "history": "^4.7.2",


What's the reasoning for downgrading?

When I ran '--legacy-peer-deps' for 'connected-react-router' to work with the React and React Router upgrade it downgraded it. The 'history' prop in the Redux store and dependency will be removed in the next ticket when the React upgrade is complete because it will no longer be supported.

npauzenga · 2025-02-14T20:23:20Z

app/src/js/components/ReconciliationReports/CreateNewReportButton.js

+import PropTypes from 'prop-types';
+import LoadingEllipsis from '../LoadingEllipsis/loading-ellipsis';
+
+const CreateNewReportButton = ({ createReportInflight, getPersistentQueryParams }) => {


Was this extracted from another component? Or is this new functionality?

Ah I see this in list.js. It looks like this is a refined version of what was rendered there previously. The tags have been updated, are there functional/visual changes here?

No visual changes just making sure the hooks work properly. Links and buttons for routing work differently because we have to call the React Router and Redux hooks. It's a cleaner way as well to handle React components.

npauzenga · 2025-02-14T20:26:03Z

app/src/js/components/ReconciliationReports/backup-report.js

+    withConflicts = [],
+    onlyInCumulus = [],
+    onlyInOrca = []
+  } = granules;


Wouldn't granules always be an empty object here?

npauzenga · 2025-02-14T20:30:34Z

app/src/js/components/Rules/overview.js

-          </div>
+    const rulesOps = {
+      list: rules.list,
+      enabled: rules.enabled || {},


Is an object the correct type to assign if rules.enabled is false? Shouldn't it be false? Same question for the next two...

npauzenga · 2025-02-14T20:31:46Z

app/src/js/components/Rules/rule.js

  }
-}
+


Another big block that looks like a move. Is there anything changed in here?

I had to refactor from class to functional component to implement React Router. No functionality changes just changed to standard. They have to be in a particular call order for React. The useEffect has to go first and all...then useMemo...

npauzenga · 2025-02-14T20:35:10Z

app/src/js/components/Sidebar/sidebar.js

-                      search: locationQueryParams.search[path] || getPersistentQueryParams(routeLocation),
-                    })}
+                    to={location}
+                    onClick={(e) => {


Does this fix an issue? Was there a conflict here where e.g. clicking the link was doing something unexpected that needed to be prevented?

npauzenga · 2025-02-17T15:48:40Z

app/src/js/components/SortableTable/SortableTable.js

-  const [leftScrollButtonVisibility, setLeftScrollButtonVisibility] = useState({ display: 'none', opacity: 0 });
-  const [rightScrollButtonVisibility, setRightScrollButtonVisibility] = useState({ display: 'none', opacity: 0 });
-  let sortByState;
+


It seems like there are a lot of changes here unrelated to the React Router update. Can you give a quick overview of the objective here?

It's still there. I just put state together and moved the let with the other variables. Some of our code is disorganized and I needed to clean it up to understand where everything is. Sorry for any confusion.

npauzenga · 2025-02-17T15:51:50Z

app/src/js/components/Table/Table.js

@@ -1,7 +1,6 @@
-import React, { useState, useEffect, lazy, Suspense, useCallback, useRef } from 'react';
+import React, { useState, useEffect, useMemo, lazy, Suspense, useCallback, useRef } from 'react';


Similar question on this file. There seem to be a lot of changes here related to memoization on tables. Was this change related to the Router update (or a knock-on effect of that update)? What drove these changes?

Because we have links to routes in our tables I had to be able to use the hooks for React Router then I had to memorize the hooks so the queries wouldn't endlessly loop.

npauzenga · 2025-02-17T15:55:22Z

app/src/js/components/oauth.js

 import { Helmet } from 'react-helmet';
+import Modal from 'react-bootstrap/Modal';
+import PropTypes from 'prop-types';
+// import get from 'lodash/get';


Reminder to remove commented code

Darn! Thought I got them all. So many files. Lol

Removed. Changes committed.

npauzenga · 2025-02-17T15:56:31Z

app/src/js/components/oauth.js

+      let state = {};
+      try {
+        state = JSON.parse(decodeURIComponent(stateParam));
+        console.log('Parsed state:', state);


Does the dash have debugging set up? Can we have log stuff like this only if e.g. the user has debugging set to "1"?

Not that I know of. I don't see much on the dashboard but we should have it though. I wanted to make sure since this is authentication. I will set a variable for that level.

npauzenga · 2025-02-17T15:58:09Z

app/src/js/components/oauth.js

+        state = JSON.parse(decodeURIComponent(stateParam));
+        console.log('Parsed state:', state);
+      } catch (error) {
+        console.error('Error parsing state:', error);


What would cause an error parsing the state? An invalid param? If that does happen, should the user be presented with an error?

Yes, if the state parameter contains an invalid URL or param. I would think so but that would be another mechanism outside of console debugging.

npauzenga · 2025-02-17T16:26:20Z

app/src/js/components/oauth.js

+  urlHelper: PropTypes.shape({
+    location: PropTypes.object,
+    navigate: PropTypes.func,
+    // historyPushWithQueryParams: PropTypes.func,


Reminder to remove comments

Doh! Good catch.

Removed. Change committed.

npauzenga · 2025-02-17T16:28:37Z

app/src/js/utils/format.js

@@ -302,6 +302,7 @@ export const providerLink = (provider) => {
 export const bool = (value) => (value ? 'Yes' : 'No');

 export const displayCase = (string) => {
+  if (!string) return nullValue;


Does this fix a bug?

I would say "yes" and making sure if something comes up undefined or empty as a check for the link. I saw it throughout this file so I made sure it was here too.

npauzenga · 2025-02-17T17:31:37Z

app/src/js/utils/table-config/collections.js

@@ -73,9 +60,10 @@ export const tableColumns = [
  {
    Header: 'MMT',
    accessor: 'MMTLink',
-    Cell: ({ cell: { value } }) => (value ? <a href={value} target="_blank">MMT</a> : null), // eslint-disable-line react/prop-types
+    Cell: ({ cell: { value } }) => (value ? <a href={value} target="_blank" rel="noopener noreferrer">MMT</a> : null), // eslint-disable-line react/prop-types


What does this do? Curious about the behavior changes here.

rel="noopener noreferrer" is a security best practice for target="_blank" and just noticed it when I was updating the links for the table.

npauzenga · 2025-02-17T17:34:25Z

app/src/js/utils/table-config/granules.js

+      const collectionId = row.collectionId || row.original?.collectionId;
+      const granuleId = row.granuleId || row.original?.granuleId;
+
+      if (collectionId && granuleId) {


Does this fix issues where collectionId isn't provided?

npauzenga · 2025-02-17T17:35:18Z

app/src/js/utils/table-config/pdrs.js

@@ -106,12 +117,16 @@ export const granuleTableColumns = [
  {
    Header: 'Name',
    accessor: 'granuleId',
+    isLink: true,


General question: Was the table link handling changed to support the router update?

npauzenga · 2025-02-17T17:36:46Z

app/src/js/utils/table-config/reconciliation-reports.js

+      const pathname = `/reconciliation-reports/report/${encodeURIComponent(value)}`;
+      const search = getPersistentQueryParams();
+
+      if (type === 'Internal' || type === 'Granule Inventory') {


Nice. Always prefer if to switches when we just have a couple cases. Nice refactor.

npauzenga · 2025-02-17T17:37:35Z

app/src/js/utils/table-config/rules.js

-  return nonEsFields;
-};
-
-export const bulkActions = (rules) => [{


Why are these removed?

I couldn't use the React Router hooks in this utility because it wasn't a React component so I moved that code to the Rules overview.js component (similar in another component that was already set up this way previously) and refactored them. Sorry for the alarm.

npauzenga · 2025-02-17T17:39:16Z

app/src/js/withUrlHelper.js

@@ -0,0 +1,87 @@
+/* eslint-disable import/no-cycle */


I don't think we need this, do we? There shouldn't be cyclic dependencies if this is a module we're creating and we've only imported third-party modules?

I will test again and see.

Removed. Tested. Looks good. Change committed.

npauzenga · 2025-02-17T17:40:14Z

cypress/integration/main_page_spec.js

@@ -60,26 +60,53 @@ describe('Dashboard Home Page', () => {
    });

    it('displays a compatible Cumulus API Version number', () => {
-      const apiVersionNumber = 'a.b.c';
+      const apiVersionNumber = 'a.b.c-d.e';


Did we change the way the version number is displayed?

Yes, it was changed in a previous ticket.

npauzenga · 2025-02-17T17:41:01Z

cypress/integration/main_page_spec.js

+      });
+    });
+
+    it('displays metrics overview with default 24-hour time range', () => {


I'm surprised to find new assertions like this as part of the React router update. Is this related?

No, that's not in result of my ticket. There was a previous ticket regarding the time range change for the Metrics Overview. I ran into the same issue with the API version. A previous change to the component that wasn't updated in the test. So...the responsibility is mine now.

npauzenga · 2025-02-17T17:48:03Z

test/components/collections/list.js

@@ -57,20 +57,42 @@ const locationQueryParams = {
  search: {}
 };

+const urlHelper = {


Why do we need to mock the urlHelper? What would be the problem if we imported the actual module?

I thought it was the better way to integrate it with what was here since the Collection List component uses urlHelper as a prop.

npauzenga · 2025-02-17T17:49:12Z

test/components/granules/granule.js

@@ -16,25 +16,62 @@ import { GranuleOverview } from '../../../app/src/js/components/Granules/granule

 const logs = { items: [''] };

-const match = { params: { granuleId: 'my-granule-id' } };
+// const match = { params: { granuleId: 'my-granule-id' } }; // removed because useParams hook now


Reminder to remove comments

Removed. Changes committed.

npauzenga · 2025-02-17T17:50:46Z

test/components/granules/granule.js

-      workflowOptions={[]}
-    />
-    </MemoryRouter>
+      <MemoryRouter initialEntries={['/granules/my-granule-id']}>


This looks like something we could store as a variable and use both here and in the previous assertion?

jjmccoy added 3 commits September 25, 2024 09:03

init PR

a66abd6

upgrade react router to v6.26.2

739d0f4

update App.js routing syntax

2c0ed05

jjmccoy added the Work In Progress label Sep 26, 2024

jjmccoy added 2 commits September 26, 2024 10:38

establish withRouter wrapper

293cc57

replace switch with routes syntax

53d8d78

jjmccoy marked this pull request as ready for review September 27, 2024 17:21

jjmccoy added 3 commits September 27, 2024 17:27

upgrade history dependency

610f753

correct root reference for router

5d44d14

create search query params wrapper for React hook

3d1d72d

jjmccoy added blocked Work In Progress and removed Work In Progress blocked labels Oct 7, 2024

jjmccoy added 16 commits October 7, 2024 14:22

establish new queryparams and useselector hooks

34b8594

rework nested routes with login and auth to react router v6

e4ce414

establish new UrlHelper HOC for query params

2eb453f

fix router and location for auth to home page

cc6b48d

consolidate HOC wrappers into one

f9cfced

parent pages params to state routed

271d4dc

initial routing updates for child pages

a5e31c3

link parent and child page routes for top navigation

e232f64

initial table config link and button params

7a2b8f8

refactor SortableTable and select table-configs

689dffe

more table routing for links and buttons

aa7e3ed

addtl routing updates

0ddab3f

more routing updates and console error fixes

c43285c

Merge branch 'develop' into CUMULUS-3862

34f4bfa

update changelog

3010f1d

update packages with --legacy-peer-deps for react router work

77ce9ab

jjmccoy added the Needs Review Looking for a reviewer label Jan 6, 2025

jjmccoy and others added 5 commits January 23, 2025 10:35

Merge branch 'develop' into CUMULUS-3862

28ebd1c

Merge branch 'develop' into CUMULUS-3862

57939ac

fix main_page_spec test errors

904bdfb

Merge branch 'develop' into CUMULUS-3862

e06c9f4

fix lint error

adfe826

npauzenga added In Review and removed Needs Review Looking for a reviewer labels Feb 13, 2025

npauzenga requested changes Feb 14, 2025

View reviewed changes

npauzenga added the PR Feedback label Feb 14, 2025

npauzenga reviewed Feb 17, 2025

View reviewed changes

npauzenga removed the In Review label Feb 17, 2025

pr feedback code cleanup

175ef09

		@@ -1,7 +1,6 @@
		import React, { useState, useEffect, lazy, Suspense, useCallback, useRef } from 'react';
		import React, { useState, useEffect, useMemo, lazy, Suspense, useCallback, useRef } from 'react';

CUMULUS-3862: Upgrade React Router to v6 #1148

Are you sure you want to change the base?

CUMULUS-3862: Upgrade React Router to v6 #1148

Conversation

jjmccoy commented Sep 25, 2024 • edited Loading

Changes

PR Checklist

npauzenga left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjmccoy Feb 18, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jjmccoy commented Sep 25, 2024 •

edited

Loading

npauzenga left a comment •

edited

Loading

jjmccoy Feb 18, 2025 •

edited

Loading