determined icon indicating copy to clipboard operation
determined copied to clipboard

chore: migrate `react-router-dom` from v5 to v6

Open keita-determined opened this issue 2 years ago • 7 comments

Description

DET-7851

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/ verify make -C webui/react test-shared passes.

keita-determined avatar Jul 21 '22 23:07 keita-determined

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

netlify[bot] avatar Jul 21 '22 23:07 netlify[bot]

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?

trentwatt avatar Jul 25 '22 00:07 trentwatt

@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?

trentwatt avatar Jul 25 '22 00:07 trentwatt

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.

keita-determined avatar Jul 25 '22 15:07 keita-determined

@hkang1 @hamidzr @trentwatt

New Changes Since the Last Review

  • Found the alternative of Prompt
  • Fix minor bugs
    • Order of props in Router.tsx
  • 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 the paroject/1 or if the params do not exist, the page says id xxx doesnt exist or something similar
    • I was thinking to use ?? instead of assertIsDefined, but sometimes it is hard to pick a right default value with ??. For example, CommandType

Concern

  • In the current behavior with old react-router-dom and history, it automatically redirect to the default path if the path doesn't exist, such as http://localhost:3000/ -> http://localhost:3000/det. However, due to the change of both react-router-dom and history (history doesnt take basename param anymore), we have to manually add /det if we run it locally.

keita-determined avatar Aug 11 '22 21:08 keita-determined

Concern

  • In the current behavior with old react-router-dom and history, it automatically redirect to the default path if the path doesn't exist, such as http://localhost:3000/ -> http://localhost:3000/det. However, due to the change of both react-router-dom and history (history doesnt take basename param anymore), we have to manually add /det if we run it locally.

This concern is fixed!!

also fixed all bugs @trentwatt found

keita-determined avatar Aug 12 '22 07:08 keita-determined

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.

keita-determined avatar Aug 12 '22 20:08 keita-determined

I close this PR to start over.

keita-determined avatar Aug 15 '22 17:08 keita-determined