determined
determined copied to clipboard
chore: migrate `react-router-dom` from v5 to v6
Description
Test Plan
- Check all pages if router works as expected
- Check code if there's something I miss
Commentary (optional)
I followed this instruction and this
Checklist
- [ ] User-facing API changes need the "User-facing API Change" label.
- [ ] Release notes should be added as a separate file under
docs/release-notes/
. See Release Note for details. - [ ] Licenses should be included for new code which was copied and/or modified from any external code.
- [ ] If modifying
/webui/react/src/shared/
verifymake -C webui/react test-shared
passes.
Deploy Preview for determined-ui ready!
Name | Link |
---|---|
Latest commit | 992a590c6a777fe8195c7cf2ede4119921f70de1 |
Latest deploy log | https://app.netlify.com/sites/determined-ui/deploys/62f6ba2ffbadac0008539634 |
Deploy Preview | https://deploy-preview-4619--determined-ui.netlify.app/ |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site settings.
From the instructions you linked:
Instead of upgrading and updating all of your code at once (which is incredibly difficult and prone to bugs), this strategy enables you to upgrade one component, one hook, and one route at a time by running both v5 and v6 in parallel. Any code you haven't touched is still running the very same code it was before. Once all components are exclusively using the v6 APIs, your app no longer needs the compatibility package and is running on v6.
Where are we in this process? Does this PR just lay the groundwork for us to be able to upgrade components one by one? Or does it update some (or all??) of the components?
@hkang1
This one (or at least the migration PRs going forward) probably needs a pretty comprehensive test plan. Can we put together some summary spec of the existing (intended) behavior for navigation?
e.g. from project details, click on experiment. then click a tab. existing behavior in this case is that you go back to project details. is this desired?
also: useSettings
has shouldPush
param, which to my understanding means that the url-encoded setting gets it own entry in the navigation history-- is that right? different test cases involving url-encoding of settings without shouldPush
probably also need to be incorporated as well.
there have also occasionally been issues around navigating e.g, /det/resourcepool/compute-pool
and /det/resourcepool/aux-pools.
also redirecting from tensorboard, opening jupyterlab in new window. navigating back from embedded view.
anything else come to mind?
From the instructions you linked:
Instead of upgrading and updating all of your code at once (which is incredibly difficult and prone to bugs), this strategy enables you to upgrade one component, one hook, and one route at a time by running both v5 and v6 in parallel. Any code you haven't touched is still running the very same code it was before. Once all components are exclusively using the v6 APIs, your app no longer needs the compatibility package and is running on v6.
Where are we in this process? Does this PR just lay the groundwork for us to be able to upgrade components one by one? Or does it update some (or all??) of the components?
Done with the entire process, so now on v6 completely. I tried to to update it little by little but our website relies on history
library a lot instead of using NavLink
. Although we don't need to have history
library since react-router-dom v6
have it as a dependency, we rely on history
a lot, so I needed to handle differently which unables to co-exist both v5 and v6.
For example, This syntax is changed. New HistoryRouter requires new version of history
.
That's why I updated all from v5 and v6 at once. However, as @trentwatt said, we need to have comprehensive test cases to upgrade safely. I think I (or someone else) should write tests first as a different PR, and then test this PR would be good.
@hkang1 @hamidzr @trentwatt
New Changes Since the Last Review
- Found the alternative of
Prompt
- Fix minor bugs
- Order of props in
Router.tsx
- Order of props in
- Added
assertIsDefined
to check if params are defined (it is mostly for typescript type checking)- Even though the code has
throw Error
, it actually doesnt run since if the params are missing, page is redirected to theparoject/1
or if the params do not exist, the page saysid xxx doesnt exist
or something similar - I was thinking to use
??
instead ofassertIsDefined
, but sometimes it is hard to pick a right default value with??
. For example,CommandType
- Even though the code has
Concern
- In the current behavior with old
react-router-dom
andhistory
, it automatically redirect to the default path if the path doesn't exist, such ashttp://localhost:3000/
->http://localhost:3000/det
. However, due to the change of bothreact-router-dom
andhistory
(history
doesnt takebasename
param anymore), we have to manually add/det
if we run it locally.
Concern
- In the current behavior with old
react-router-dom
andhistory
, it automatically redirect to the default path if the path doesn't exist, such ashttp://localhost:3000/
->http://localhost:3000/det
. However, due to the change of bothreact-router-dom
andhistory
(history
doesnt takebasename
param anymore), we have to manually add/det
if we run it locally.
This concern is fixed!!
also fixed all bugs @trentwatt found
I'll fix the issue around password. Thanks for finding
The issues around password is fixed.
It was unclear to me from your https://github.com/determined-ai/determined/pull/4619#issuecomment-1194191120 whether that was impossible, or just somehow difficult.
I followed the instruction step by step, but at some point, project got broken. We heavily use history
, so I assume it might be the reason why it is difficult to upgrade step by step. I won't say it's impossible but IMO it is hard to gradually upgrade.
Side question: what is the motivation for the upgrade in the first place? is v5 nearing end of support?
The motivation is that lots of libraries around React is not updated for several months. The longer we wait to update libraries, the more difficult it will be in the future. Especially, React 18 and React-Router-Dom v6 have some breaking changes.
I close this PR to start over.