-
Notifications
You must be signed in to change notification settings - Fork 487
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
Upgrade react-router-dom to latest version #1825
Comments
|
Yes, we would have to upgrade it sequentially version by version, keeping the project intact |
@anshgoyalevil do we have to migrate it to v5 before v6? |
Yes. First v5, then 5.1, and then v6. (There can be more versions between 5.1 and 6, to which we would have to upgrade first). Though it is a part of my LFX Mentorship, if you want to help out, you may DM me on Slack, and we can collaborate on another dependency upgrade, so that we both don't have to create the same type of PRs simultaneously. |
## Which problem is this PR solving? - related to: #1825 - The `react-router-redux` is a dead project and is not compatible with `react-router-dom` `v5` and `v6`. - The whole react-router community suggested using `redux-first-history` which is compatible with `v4`, `v5` and `v6` both. ## How was this change tested? - Manually along with redux dev toolkit and automated testing ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <[email protected]>
## Which problem is this PR solving? - part of: #1825 - Upgrades react-router-dom to v5.2.0 ## Description of the changes - This PR upgrades the rrd to v5.2.0 - This upgrade was previously attempted with PR #727 but at that time, an issue (#803) was reported because `react-router-redux` v5.x was not compatible with rrd v5.2.0, so it was reverted with PR: #837 - Now, since we have `redux-first-history` instead of `react-router-redux`, we can upgrade to rrd v5.2.0 safely now. ## How was this change tested? - The reported issue with v5.2.0 (#803) is not being reproduced now. ![image](https://github.com/jaegertracing/jaeger-ui/assets/94157520/43c11ec6-02e6-4ede-855b-22822f77d4ae) ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <[email protected]>
…1970) ## Which problem is this PR solving? - part of: #1825 ## Description of the changes - This PR adds the react-router compatibility package to merge v5 and v6 APIs according to the official migration guide: remix-run/react-router#8753 and https://reactrouter.com/en/6.16.0/upgrading/v5 - After this PR, we need to change `<Route>` to `<CompatRoute>` one at a time, to minimize the impact area per patch. ## How was this change tested? - `yarn start` + `yarn test` ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <[email protected]>
This is a Tracker Issue for all react-router-dom related upgrades.
Route
defined insideindex.jsx
to use v6 API.elements
attribute. Example commit here.<Outlet />
instead ofprops.children
to render nested routes inside<Page />
. Example commit here.Useful resources:
<Outlet />
and<Switch>
components: ff22e64 (This commit was created as a part of the migration but is just a part of the story since it doesn't fixes the nested routing APIs and also doesn't use v6 Link component)The text was updated successfully, but these errors were encountered: