Skip to content

Replace react-router-dom with react-router #4540

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

Merged
merged 3 commits into from
Jan 18, 2025

Conversation

mproffitt
Copy link
Contributor

What changed?

This commit rewrites much of the routing to support react-router instead of react-router-dom which is deprecated/removed in version 7.

This change moves the router across 2 major versions 5.2.0 to 7.1.1

When moving from react-router-dom 5.2.0 to version 6.x, largescale changes were made to how routing and navigation was managed. The change from 6.x to 7.x further refined this, changing how sub-routes were handled by forcing a split of the Route(/*) into Route -> Route(*)

This split introduced breaking changes into how routing is managed by the application.

The change to router also had impacts on how automatic logout was managed as this prevented the sign-in page from completely loading, resulting in a loading icon replacing the continue button.

Every care has been taken to ensure all routes and tabs are functional however this is not currently exhaustive.

Why was this change made?

To ensure ongoing and future compatibility with upstream libraries.

How was this change implemented?

By working through the routing to ensure it was upgraded to match [email protected]

How did you validate the change?

By loading the UI using both yarn start and tilt up and ensuring in both environments all visible pages and tabs are accessible.

By ensuring that all relevant unit tests pass with minimal difference.

One test suite was modified by this change. The Link test suite contained invalid / broken test cases that became apparent as a result of the change being made. These have now been corrected for URLs treated as relative links.

Release notes

N/A

Documentation Changes

N/A

@mproffitt mproffitt force-pushed the rewrite-router branch 2 times, most recently from 4db2c06 to 054db59 Compare January 16, 2025 08:53
casibbald
casibbald previously approved these changes Jan 16, 2025
Copy link
Collaborator

@casibbald casibbald left a comment

Choose a reason for hiding this comment

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

I have tested the branch, and UI and elements are functional

@mproffitt
Copy link
Contributor Author

Found one broken link to the image repository and removed an accidentally included test suite change

@casibbald casibbald self-requested a review January 16, 2025 14:36
casibbald
casibbald previously approved these changes Jan 16, 2025
Copy link
Collaborator

@casibbald casibbald left a comment

Choose a reason for hiding this comment

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

Update tested

@erikgb erikgb requested a review from tenstad January 16, 2025 19:40
Copy link
Contributor

@gusevda gusevda left a comment

Choose a reason for hiding this comment

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

Great job! I left some comments, most important are in the <SubRouterTabs /> component. Overall looks very good!

gusevda
gusevda previously approved these changes Jan 17, 2025
Copy link
Contributor

@gusevda gusevda left a comment

Choose a reason for hiding this comment

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

Awesome, thanks!

dependabot bot and others added 2 commits January 17, 2025 19:58
This commit rewrites much of the routing to support react-router instead
of react-router-dom which is deprecated/removed in version 7.

react-router-dom 6 introduced large changes to how navigation was
managed. This is compounded by layout changes inside react-router 7.

This change addresses the need by reworking the navigation to match the
format specified by react-router 6 and 7
- Drop types/react-router
- Set all routes to be element instead of Component
- Undo convertImage change moved to separate PR
- Review comments offer simplification to SubRouterTabs
- Fix broken Automations route
- Reset and reapply change to AppContext to cleanup  redundant changes
- As above for CoreContext
- Remove typecasting, prettify, lint
@mproffitt mproffitt merged commit 0feb495 into weaveworks:main Jan 18, 2025
18 checks passed
@mproffitt mproffitt deleted the rewrite-router branch January 18, 2025 09:31
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