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

feat(components, app, labware-library): migrate to react-router-dom v6.24.1 #15699

Merged
merged 12 commits into from
Jul 22, 2024

Conversation

jerader
Copy link
Collaborator

@jerader jerader commented Jul 17, 2024

closes AUTH-564

Overview

This PR updates react-router-dom to v6.24.1 in the stack to match PD's react-router-dom version

Big changes between v5 and v6:

  1. useHistory is deprecated and replaced with useNavigate. Instead of history.push('path') and history.goBack() it is navigate('path') and navigate(-1)
  2. Route no longer has children, instead the component goes into the element prop.
  3. Switch is deprecated and is now Routes
  4. Route must be wrapped in Routes (should mainly have to worry about this in unit testing since the app components are wrapped in Routes already from the top level)
  5. <Redirect/> is deprecated and is now <Navigate/> (if it is wrapped in Routes then you may use Route and Navigate goes into the element prop)

Test Plan

Smoke test labware-library, protocol designer but most importantly, smoke test the app and ODD.

Changelog

  • update react-router-dom in Labware-library and app
  • deprecate it in components
  • fix all affected components and tests

Review requests

see test plan

Risk assessment

high since we have a 8.0.0 release and this touches a bunch of code. we need to smoke test well before merging

@jerader jerader force-pushed the chore_update-react-router-dom branch from 964390c to 3928521 Compare July 17, 2024 21:37
@jerader jerader marked this pull request as ready for review July 17, 2024 21:37
@jerader jerader requested review from a team as code owners July 17, 2024 21:37
@jerader jerader requested review from ncdiehl11, koji, shlokamin, smb2268, brenthagen and mjhuff and removed request for a team July 17, 2024 21:37
@koji koji requested a review from sfoster1 July 17, 2024 21:43
app/src/index.tsx Outdated Show resolved Hide resolved
@koji
Copy link
Contributor

koji commented Jul 17, 2024

the migration guide would be useful to review this pr
remix-run/react-router#8753

@@ -53,7 +53,7 @@
"react-intersection-observer": "^8.33.1",
"react-markdown": "9.0.1",
"react-redux": "8.1.2",
"react-router-dom": "5.3.4",
"react-router-dom": "6.24.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

not introducing v6.25.0/v6.25.1 because too new?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh wait i think v6.25.0 came out just yesterday, that is why i did not use it lol. Shouldn't we stick with 6.24.1 to match PD though? hmm. @shlokamin thoughts?

Copy link
Contributor

@koji koji Jul 18, 2024

Choose a reason for hiding this comment

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

6.25.0 2days ago and 6.25.1 yesterday.
https://github.com/remix-run/react-router/releases

Copy link
Member

Choose a reason for hiding this comment

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

doesn't matter to me as long as all of our frontend projects share the same version, so we can either update PD or keep these at 6.24.1

Comment on lines 268 to 270
{targetPath != null && (
<Route path="/" element={<Navigate to={targetPath} />} />
)}
Copy link
Contributor

Choose a reason for hiding this comment

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

https://stackoverflow.com/questions/69868956/how-can-i-redirect-in-react-router-v6

Suggested change
{targetPath != null && (
<Route path="/" element={<Navigate to={targetPath} />} />
)}
<Route path="*" element={<Navigate to={targetPath} replace />} />

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thank you so much koji!

@koji
Copy link
Contributor

koji commented Jul 19, 2024

For ODD, we would need to check the followings
unboxing flow [without software update]
welcome -> network setup -> software update -> e-stop screen -> name robot -> finish setup -> robot dashboard with modal

unboxing flow [with software update]
welcome -> network setup -> software update -> reboot the robot -> e-stop screen -> name robot -> finish setup -> robot dashboard with modal

Navigation item after unboxing flow

  • protocols
  • quick transfer
  • instruments
  • settings

Robot settings Dashboard

  • network setup flow
  • rename flow

@shlokamin
Copy link
Member

making my way through this PR but chiming in to say that i can devote time early next week to test this on a bot!

@koji koji self-requested a review July 19, 2024 21:00
Copy link
Contributor

@koji koji left a comment

Choose a reason for hiding this comment

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

Tested this branch on ODD (tested the UnboxingFlow 2,3 times a couple of times) and didn't see any issues.
I think once Shlok tested this branch and if he doesn't see any issue, this branch will be good to go!

Thank you for updating react-router!

@jerader jerader merged commit 6e1dca6 into edge Jul 22, 2024
54 checks passed
@jerader jerader deleted the chore_update-react-router-dom branch July 22, 2024 18:22
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.

3 participants