Upgrade react-router to v6 #3261

nickclyde · 2022-01-21T23:42:23Z

Related Issue or Background Info

Continuing Dependency updates: React #2908

Changes Proposed

Update react-router-dom to version 6.2.1. The major version upgrade from v5 to v6 introduced a number of breaking changes that touch a lot of different parts of the app. I followed this upgrade guide, making the following changes:
- Switch to the hooks API introduced in v5.1 so that we don't have to pass the match prop down to child routes to access things like location and params. This is a huge improvement for code cleanliness!
- <Route component={SomeComponent}> /> now becomes <Route element={<SomeComponent />} />, which allows us to easily pass props into routes! More info on the benefits of this here
- Replace all uses of withRouter with hooks
- Convert all <Redirect> components into <Navigate> components
- Refactor the <GuardedRoute> and <ProtectedRoute> components into ones that get passed into an element prop of a <Route />, rather than returning a <Route /> component themselves. You can read a little more about why this change was made here.
- Upgrade all <Switch> elements to <Routes> - see here, but the new <Routes> component allows relative routing!
- Start using relative routes and links. You may have noticed before that some of our app components like PatientApp used to include an additional <Router> component with a basename prop so that we didn't have to prefix every route with /pxp. With v6 that's no longer needed because <Route> components can now be nested and the paths always include the parent route's path! The same goes for <Link> and <Navigate> components.
- <Route> paths no longer need a leading slash, and can end in a wildcard * to match all subpaths. More info
- Convert all uses of useHistory to useNavigate. More info
- Related to the above, since useHistory is no longer available, we can't pass history into our Application Insights config. However, I don't think it was needed in the first place according to this comment. Since we have enableAutoRouteTracking: true set, it should already record any route changes automatically (see docs here)
- Remove activeClassName and activeStyle props from Links
- Last, the react-router team removed support for the <Prompt> component from this release in order to expedite the release. However, I was able to use a reimplementation found in this GH issue to keep that functionality since we use it in a couple places in the app. However, the team plans to readd it at some point, so I will keep an eye out and be sure to switch to the official API when it comes back.
These changes broke a lot of our tests, so I had to change a bunch of the tests, especially the ones that use <MemoryRouter>
Remove the react-router dependency. The react-router package includes everything from both react-router-dom and react-router-native, but we only use react-router-dom. Also updated all imports across the app to use react-router-dom
Added a REACT_APP_DISABLE_MAINTENANCE_BANNER environment variable that disables the maintenance banner, because it was causing issues on the e2e tests.
Added a /reload-app route which just redirects back to /. This allows us to retrigger the whoami query (for example after spoofing into an org) without actually refreshing the page and having to reload the bundle.

Additional Information

Another change that would be interesting is to use the <Outlet> component (docs here). This would allow us to place all of our <Route> components into a single tree in index.tsx, and then use the <Outlet> component in <Route> elements to refer to the child routes. This would be another big win for code cleanliness!
- I took a stab at implementing, but wasn't sure how to pass state back up to the parent component, so I'm going to skip this for now.

Screenshots / Demos

Checklist for Author and Reviewer

Infrastructure

Consult the results of the terraform-plan job inside the "Terraform Checks" workflow run for this PR. Confirm that there are no unexpected changes!

Design

Any UI/UX changes have a designer as a reviewer, and changes have been approved
Any large-scale changes have been deployed to test, dev, or pentest and smoke-tested by both the engineering and design teams

Content

Any content changes (including new error messages) have been approved by content team

Support

Any changes that might generate new support requests have been flagged to the support team
Any changes to support infrastructure have been demo'd to support team

Testing

Includes a summary of what a code reviewer should verify

Changes are Backwards Compatible

Database changes are submitted as a separate PR
- Any new tables that do not contain PII are accompanied by a GRANT SELECT to the no-PHI user
- Any changes to tables that have custom no-PHI views are accompanied by changes to those views
  (including re-granting permission to the no-PHI user if need be)
- Liquibase rollback has been tested locally using ./gradlew liquibaseRollbackSQL or liquibaseRollback
- Each new changeset has a corresponding tag
GraphQL schema changes are backward compatible with older version of the front-end

Security

Changes with security implications have been approved by a security engineer (changes to authentication, encryption, handling of PII, etc.)
Any dependencies introduced have been vetted and discussed

Cloud

DevOps team has been notified if PR requires ops support
If there are changes that cannot be tested locally, this has been deployed to our Azure test, dev, or pentest environment for verification

emmastephenson

Overall LGTM, thanks for putting so much work into this! Just a couple questions and comments.

emmastephenson · 2022-01-27T16:35:33Z

frontend/src/app/App.tsx

+                <ProtectedRoute
+                  requiredPermissions={appPermissions.people.canView}
+                  userPermissions={data.whoami.permissions}
+                  element={<ManagePatientsContainer />}


I wonder if it's worth defining this protected route as a variable, since it's repeated under the regular patients route too.

Yeah not a bad idea! The reason I had to duplicate this route is because v5 supported optional URL params like /patients/:pageNumber? which would match both /patients and /patients/1, but v6 no longer supports that. Hence the need for two routes. But I will definitely DRY it up

Soooo I tried to do this, and for some reason it broke the patients page. It kept throwing "The operation is insecure", and I couldn't figure out why 🤔 so I reverted for now.

emmastephenson · 2022-01-27T16:37:32Z

frontend/src/app/App.tsx

+              element={
+                <ProtectedRoute
+                  requiredPermissions={appPermissions.settings.canView}
+                  userPermissions={data.whoami.permissions}


nit: we may want to extract this data.whoami.permissions out to a variable

Same as above - moving to a shorter named const seemed to cause issues, so I reverted

emmastephenson · 2022-01-27T16:42:00Z

frontend/src/app/TelemetryService.ts

@@ -31,11 +30,6 @@ const createTelemetryService = () => {
        enableAutoRouteTracking: true,
        loggingLevelTelemetry: 2,
        maxBatchInterval: 0,


Can you deploy this to one of the lower environments and double-check that the telemetry still works as anticipated?

Good call, I will do that today.

Thanks! Can you let me know what the outcome was?

Just messed around in dev and checked app insights, looks like we are still receiving everything, let me know if there's anything in particular I should look for, but it's still recording all requests and our custom events:

emmastephenson · 2022-01-27T16:48:23Z

frontend/src/app/accountCreation/MfaSendCodeToContact/MfaSendCodeToContact.tsx

-      <Redirect
-        push
-        to={{
-          pathname: `${window.location.pathname.split("/uac")[1]}/verify`,


so happy to see these gone!

emmastephenson · 2022-01-27T16:50:57Z

frontend/src/app/commonComponents/Header.tsx

@@ -108,6 +108,14 @@ const Header: React.FC<{}> = () => {
    );
  };

+  const activeNavItem = "active-nav-item prime-nav-link";


emmastephenson · 2022-01-27T17:10:17Z

frontend/src/patientApp/PatientApp.tsx

+            element={
+              <GuardedRoute auth={auth} element={<AoEPatientFormContainer />} />
+            }
+          />


I think we might be able to kill this route, AFAIK we removed the ability for patients to fill out the AoE.

That's true, but I think @zdeveloper may be working on cleaning these routes out in another PR IIRC.

fzhao99

thanks again for walking me through everything!

frontend/src/app/commonComponents/Header.tsx

sonarqubecloud · 2022-01-28T21:08:27Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

81.4% Coverage
0.0% Duplication

This reverts commit 46bb4cd.

nickclyde added 20 commits January 21, 2022 10:30

Upgrade react-router to v6

3c977e2

Merge branch 'main' into nick/upgrade-react-router

1f24239

Fix tests

0cda207

Merge branch 'main' into nick/upgrade-react-router

5007567

Merge branch 'main' into nick/upgrade-react-router

86e9268

Code smells

6bb30ac

Fix various TypeScript errors

7997852

Add missing useEffect dependency

b57c3c4

Fix QueueItem storybook story

3daf66c

Undo stupid change where I confused path params with URL search params

4fe9c77

Forgot a few wildcards

713319f

Add test for Prompt

b60ef2d

Fix <Navigate> to props in AccountCreationApp

45db92a

Merge branch 'main' into nick/upgrade-react-router

3a5e30c

Make all navigation relative

414ab98

Fix tests

93a2988

Use absolute paths in places where it makes more sense

d0ad467

A few navigation bugfixes

ce65cdc

Code smells

66a434d

Add /reload-app path for reloading whoami query for TenantDataAccess

a2e0772

nickclyde requested review from zdeveloper, emmastephenson, fzhao99, jdorothy and nathancrtr January 26, 2022 00:06

nickclyde temporarily deployed to Test January 26, 2022 00:26 Inactive

nickclyde added 2 commits January 25, 2022 17:12

A little more test coverage for <Prompt>

33c7d5f

Merge branch 'main' into nick/upgrade-react-router

20256f8

emmastephenson reviewed Jan 27, 2022

View reviewed changes

Merge branch 'main' into nick/upgrade-react-router

6a3a4ab

fzhao99 reviewed Jan 27, 2022

View reviewed changes

frontend/src/app/commonComponents/Header.tsx Outdated Show resolved Hide resolved

nickclyde added 3 commits January 27, 2022 10:45

DRY up App component

fda9951

Apply style to active-nav-item class instead of using style prop

294d217

Revert App changes

594b6b6

nickclyde requested review from emmastephenson and fzhao99 January 27, 2022 21:56

nickclyde temporarily deployed to Dev January 27, 2022 22:03 Inactive

nickclyde requested review from zdeveloper, nathancrtr, emmastephenson and fzhao99 and removed request for jdorothy, zdeveloper, nathancrtr, fzhao99 and emmastephenson January 28, 2022 20:48

Merge branch 'main' into nick/upgrade-react-router

7d43416

nickclyde mentioned this pull request Jan 28, 2022

Upgrade apollo and related packages #3304

Merged

17 tasks

Merge branch 'main' into nick/upgrade-react-router

008ed0e

fzhao99 approved these changes Feb 1, 2022

View reviewed changes

emmastephenson approved these changes Feb 1, 2022

View reviewed changes

nickclyde merged commit 46bb4cd into main Feb 2, 2022

nickclyde deleted the nick/upgrade-react-router branch February 2, 2022 02:07

zdeveloper added a commit that referenced this pull request Feb 2, 2022

Revert "Upgrade react-router to v6 (#3261)"

2768b79

This reverts commit 46bb4cd.

zdeveloper mentioned this pull request Feb 2, 2022

Revert "Upgrade react-router to v6" #3333

Merged

nickclyde pushed a commit that referenced this pull request Feb 2, 2022

Revert "Upgrade react-router to v6 (#3261)" (#3333)

fdf5896

This reverts commit 46bb4cd.

This was referenced Feb 2, 2022

Upgrade react-router to v6 #3337

Merged

Change app gateway to redirect self-registration links to /app #3344

Merged

emyl3 mentioned this pull request Oct 19, 2022

Bump react-router-dom from 6.2.1 to 6.4.2 in /frontend #4508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade react-router to v6 #3261

Upgrade react-router to v6 #3261

nickclyde commented Jan 21, 2022 •

edited

Loading

emmastephenson left a comment

emmastephenson Jan 27, 2022

nickclyde Jan 27, 2022

nickclyde Jan 27, 2022

emmastephenson Jan 27, 2022

nickclyde Jan 27, 2022

emmastephenson Jan 27, 2022

nickclyde Jan 27, 2022

emmastephenson Jan 28, 2022

nickclyde Jan 28, 2022

emmastephenson Jan 27, 2022

emmastephenson Jan 27, 2022

emmastephenson Jan 27, 2022

nickclyde Jan 27, 2022

fzhao99 left a comment

sonarqubecloud bot commented Jan 28, 2022

Upgrade react-router to v6 #3261

Upgrade react-router to v6 #3261

Conversation

nickclyde commented Jan 21, 2022 • edited Loading

Related Issue or Background Info

Changes Proposed

Additional Information

Screenshots / Demos

Checklist for Author and Reviewer

Infrastructure

Design

Content

Support

Testing

Changes are Backwards Compatible

Security

Cloud

emmastephenson left a 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

fzhao99 left a comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 28, 2022

nickclyde commented Jan 21, 2022 •

edited

Loading