Skip to content

ROX-27046: react router v6 upgrade#13628

Merged
bradr5 merged 10 commits intomasterfrom
ROX-27046_react_router_v6
Mar 26, 2025
Merged

ROX-27046: react router v6 upgrade#13628
bradr5 merged 10 commits intomasterfrom
ROX-27046_react_router_v6

Conversation

@bradr5
Copy link
Copy Markdown
Contributor

@bradr5 bradr5 commented Dec 17, 2024 •
edited
Loading

Description

React Router v6 Upgrade

Key Changes

  • Switch from connected-react-router to redux-first-history

    • connected-react-router not compatible with router v6
      • router v6 handles history internally
      • replies on withRouter
    • redux-first-history is a modern replacement
      • works with @remix-run/router history object

  • Routing

    • Replace <Switch> with <Routes>
    • <Route> now uses the element prop instead of component/render or children
    • path now relative to parent route
    • Route matching is always exact by default

  • useNavigate replaces useHistory

    • Use navigate for navigation instead of history
      • push by default with optional prop {replace: true} for replace
      • history.goBack()/history.goForward() replaced by navigate(-1)/navigate(1)

  • Redirect replaced by Navigate

    • Replace <Redirect> with <Navigate to="path" /> for redirection

  • useMatch replaces useRouteMatch

    • Use useMatch to check for route matches

Issues

  • useParams Type Definitions
    • Route parameters returned from useParams are typed as string | undefined

Considerations

  • Outlet for Nested Routes

    • Use <Outlet /> in parent components to render child routes dynamically

  • useRoutes Hook

    • Define routes programmatically using the useRoutes hook

User-facing documentation

  • CHANGELOG is updated OR update is not needed
  • documentation PR is created and is linked above OR is not needed

Testing and quality

  • the change is production ready: the change is GA or otherwise the functionality is gated by a feature flag
  • CI results are inspected

Automated testing

  • added unit tests
  • added e2e tests
  • added regression tests
  • added compatibility tests
  • modified existing tests

How I validated my change

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Dec 17, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from 1b94499 to d3b2961 Compare December 18, 2024 21:41
@rhacs-bot
Copy link
Copy Markdown
Contributor

rhacs-bot commented Dec 18, 2024 •
edited
Loading

Images are ready for the commit at 7f25dec.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.8.x-320-g7f25dec30f.

pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
pedrottimark
pedrottimark reviewed Dec 20, 2024
View reviewed changes
@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from a45af79 to aeaf0dc Compare January 6, 2025 20:03
@bradr5 bradr5 marked this pull request as ready for review January 6, 2025 20:04
@bradr5 bradr5 requested a review from a team as a code owner January 6, 2025 20:04
@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from ea04091 to a13ef4a Compare January 7, 2025 21:05
@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from 62d7bd0 to 791f882 Compare March 3, 2025 05:15
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 3, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 48.89%. Comparing base (f10ea10) to head (7f25dec).
Report is 18 commits behind head on master.

Additional details and impacted files 
@@            Coverage Diff             @@
##           master   #13628      +/-   ##
==========================================
- Coverage   49.20%   48.89%   -0.31%     
==========================================
  Files        2533     2547      +14     
  Lines      185529   186992    +1463     
==========================================
+ Hits        91290    91433     +143     
- Misses      87003    88318    +1315     
- Partials     7236     7241       +5
Flag Coverage Δ
go-unit-tests 48.89% <ø> (-0.31%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dvail
dvail reviewed Mar 4, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@dvail dvail left a comment

Choose a reason for hiding this comment

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

This looks really good - thanks for all your hard work on this @bradr5 !

A handful of thoughts:

(1) Regarding the need for react-router-v5 compat in the RH console plugin architecture

I don't think we should allow that concern to hold up the work in this PR. From what I understand, v5 is deprecated and the next release will likely support v6 only. Assuming we needed to build a plugin tomorrow, which we don't, we would still need to have the plugin code working in a v6-only environment, as well as a v5/v6 environment in order to support older console versions.

Although I wasn't able to easily get react-router-dom-v5-compat working as-is with this PR, I think that the path from here to there is much shorter than the path from v5 to there.

As an additional aside, when hooking into the console we will not be using most of the top level <Router>, etc. components, as those will be provided. Using Violations as an example, aside from the <ViolationsPage> which we could easily bypass, we only need <Link> and useParams() in components in the Violations directory. The API for these hasn't changed between v5 and v6, so that should not be much of an issue. If there are other react-router APIs that we need that have changed, we could inject a wrapper via Context or some other method if we absolutely had to.
image

(2) I'm not sure if it is possible at this point, but it would be nice to inventory everything that was found in a bug bash document that did not cause an e2e test failure, and create follow-on tickets to get tests in place for those. The types of failures we found (ex. an entire page does not render) should definitely be caught by some smoke test in our suite given the severity of the failure.

(3) We should do some more testing before approval, especially with changes that landed at the end of the 4.7 cycle (VM, Violations). Aside from that, the sooner we get this in the better IMO as it will give us ample soak time and unblock other dependency work that is in the pipeline.

@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from 7f86ec0 to 6572ede Compare March 5, 2025 01:20
@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from 166ef7b to 7a70532 Compare March 17, 2025 19:49
@bradr5 bradr5 force-pushed the ROX-27046_react_router_v6 branch from 11d62cc to fe4f316 Compare March 24, 2025 17:58
@bradr5
Copy link
Copy Markdown
Contributor Author

bradr5 commented Mar 24, 2025

(1) Regarding the need for react-router-v5 compat in the RH console plugin architecture

I don't think we should allow that concern to hold up the work in this PR. From what I understand, v5 is deprecated and the next release will likely support v6 only. Assuming we needed to build a plugin tomorrow, which we don't, we would still need to have the plugin code working in a v6-only environment, as well as a v5/v6 environment in order to support older console versions.

Although I wasn't able to easily get react-router-dom-v5-compat working as-is with this PR, I think that the path from here to there is much shorter than the path from v5 to there.

As an additional aside, when hooking into the console we will not be using most of the top level , etc. components, as those will be provided. Using Violations as an example, aside from the which we could easily bypass, we only need and useParams() in components in the Violations directory. The API for these hasn't changed between v5 and v6, so that should not be much of an issue. If there are other react-router APIs that we need that have changed, we could inject a wrapper via Context or some other method if we absolutely had to.
image

(2) I'm not sure if it is possible at this point, but it would be nice to inventory everything that was found in a bug bash document that did not cause an e2e test failure, and create follow-on tickets to get tests in place for those. The types of failures we found (ex. an entire page does not render) should definitely be caught by some smoke test in our suite given the severity of the failure.

(3) We should do some more testing before approval, especially with changes that landed at the end of the 4.7 cycle (VM, Violations). Aside from that, the sooner we get this in the better IMO as it will give us ample soak time and unblock other dependency work that is in the pipeline.

  1. I appreciate you bringing up "plugin" support in the first place, it's something I never even considered. I'm really just hoping that by the time that comes (assuming it does) v6 will be supported as is.
  2. Another great idea... I'll also try to go through and remember everything I caught manually prior to the bug bash and add it to the doc.
  3. I'll setup another bug bash.

dvail
dvail approved these changes Mar 26, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@dvail dvail left a comment

Choose a reason for hiding this comment

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

Code LGTM and the testing has been pretty solid. I think there is still something a little bit amiss with the handling of isActive states of nav items as you navigate through the pages. I don't think it is a critical bug but we should prioritize it as a first fix after this is merged if we want to get this in asap.

@bradr5 bradr5 merged commit 244e2ab into master Mar 26, 2025
91 checks passed
@bradr5 bradr5 deleted the ROX-27046_react_router_v6 branch March 26, 2025 18:52
@bradr5
Copy link
Copy Markdown
Contributor Author

bradr5 commented Mar 27, 2025

@sourcery-ai review

@dvail dvail mentioned this pull request Jul 17, 2025
Merged
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dvail dvail dvail approved these changes

@pedrottimark pedrottimark pedrottimark approved these changes

Assignees

No one assigned

Labels

area/ui

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bradr5 @rhacs-bot @dvail @pedrottimark