Upgrade react-router to v6 #3337
Conversation
|
Fixed the three bugs that were found when #3261 was merged to prod:
Video: router-bugs-fixed.mp4 |
|
Also deploying to dev for more thorough smoke testing. |
There was a problem hiding this comment.
Overall the new changes look great! Smoke tested the following in dev and all worked well:
- patient self-reg
- patient results (texting works and opening link works)
- pagination on results
- pagination on people
- user activation flow
- signup flow
- support admin flow
The one thing that looked off to me was the sub-nav for admins - it doesn't clear off "manage users" correctly when you nav over to "manage facilities"
That's the only thing I can see that needs to be changed though!
| @@ -62,6 +61,14 @@ describe("ManagePatients", () => { | |||
| expect(await screen.findByText(patients[1].lastName, { exact: false })); | |||
| expect(await screen.findByText(patients[2].lastName, { exact: false })); | |||
| }); | |||
| it("can go to page 2", async () => { | |||
There was a problem hiding this comment.
Thanks for adding a test here 👍
| @@ -85,38 +92,248 @@ describe("ManagePatients", () => { | |||
|
|
|||
| const patients = [ | |||
There was a problem hiding this comment.
(no-op) Holy cow, thanks for adding all these patients. Makes me want to up the priority of this ticket: #2929
|
Kudos, SonarCloud Quality Gate passed! |
| newOrg?: boolean; | ||
| } | ||
|
|
||
| const FacilityFormContainer: any = (props: Props) => { | ||
| const { facilityId } = useParams(); |
There was a problem hiding this comment.
Any reason why is this being grabed from query params and not the props
There was a problem hiding this comment.
this is what the url look like in test
https://test.simplereport.gov/app/settings/facility/7f1d9789-37b0-4a83-99fa-824403e95e90?facility=08ea10c8-a767-4d72-a60a-46dca51228bc
I see key of facility and not facilityId
There was a problem hiding this comment.
So in that case, the uuid after /facility is the id of the facility being edited in the form, whereas the facility query param refers to the currently selected facility in the nav bar. We want the id from the url params because that's the one we actually want to edit in the form.
There was a problem hiding this comment.
That makes sense, I wish it was a bit clearer which facility is which, but that could be addressed in a future refactor.
Related Issue or Background Info
Changes Proposed
react-router-domto version6.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:matchprop 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 herewithRouterwith hooks<Redirect>components into<Navigate>components<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.<Switch>elements to<Routes>- see here, but the new<Routes>component allows relative routing!PatientAppused to include an additional<Router>component with abasenameprop 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 infouseHistorytouseNavigate. More infouseHistoryis 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 haveenableAutoRouteTracking: trueset, it should already record any route changes automatically (see docs here)<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.<MemoryRouter>react-routerdependency. Thereact-routerpackage includes everything from bothreact-router-domandreact-router-native, but we only usereact-router-dom. Also updated all imports across the app to usereact-router-domREACT_APP_DISABLE_MAINTENANCE_BANNERenvironment variable that disables the maintenance banner, because it was causing issues on the e2e tests./reload-approute which just redirects back to/. This allows us to retrigger thewhoamiquery (for example after spoofing into an org) without actually refreshing the page and having to reload the bundle.Additional Information
<Outlet>component (docs here). This would allow us to place all of our<Route>components into a single tree inindex.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!Screenshots / Demos
Checklist for Author and Reviewer
Infrastructure
terraform-planjob inside the "Terraform Checks" workflow run for this PR. Confirm that there are no unexpected changes!Design
test,dev, orpentestand smoke-tested by both the engineering and design teamsContent
Support
Testing
Changes are Backwards Compatible
(including re-granting permission to the no-PHI user if need be)
./gradlew liquibaseRollbackSQLorliquibaseRollbackSecurity
Cloud
test,dev, orpentestenvironment for verification