Improve routes handling #1192
Improve routes handling #1192
Conversation
src/Routes.js
Outdated
| <Routes> | ||
| <Route | ||
| element={ | ||
| <PermissionRoute requiredPermissions={generalPermissions} /> |
There was a problem hiding this comment.
I'm not to sure about doing the permission handling this way. I'd rather have a plain component that renders or not renders the children passed, rather than utilising react-router and it's "Outlet".
There was a problem hiding this comment.
Yeah, completely agree. Refactored in the last commit.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1192 +/- ##
==========================================
- Coverage 64.01% 63.97% -0.04%
==========================================
Files 124 125 +1
Lines 3235 3237 +2
Branches 831 833 +2
==========================================
Hits 2071 2071
- Misses 1164 1166 +2 ☔ View full report in Codecov by Sentry. |
src/Routes.js
Outdated
| @@ -109,41 +90,35 @@ const PatchRoutes = () => { | |||
| </Suspense> | |||
| ) : ( | |||
| <Routes> | |||
| <Route path="/advisories" element={<Advisories />} /> | |||
There was a problem hiding this comment.
I don't know why it work here with absolute paths, but these should be relative. All other applications use relative paths for their routes[0].
However, I think we should try to make these relative as well and also make sure we consistently use the insights navigate stuff, so links resolve properly.
[0] https://github.com/RedHatInsights/compliance-frontend/blob/master/src/Routes.js#L23
There was a problem hiding this comment.
I don't know why it work here with absolute paths
It simply matches the routes starting by the basename (which is / in our case because /insights/patch pathname prefix is cut by chrome/federated modules related reason). Navigating to /advisories will match with the route having a path /advisories. As well as it will match the route with just advisories path. In our case, it doesn't have any disadvantages but inconsistency with other apps, as you mentioned. Though, all apps are inconsistent, Inventory has the same "root slash preference": https://github.com/RedHatInsights/insights-inventory-frontend/blob/master/src/Routes.js.
There was a problem hiding this comment.
I would say these paths starting with a slash are actually more specific, they are fully matching the route. Though, if for example path="advisories" would be by accident placed under some other route, it will lead to the situation where any path that has some slices and ends with "advisories" will match. It's very in theory though, but just imagining.
Do you think it's worth consolidating?
There was a problem hiding this comment.
I think this here works maybe only because of some trickery we do in the routing in chrome, but I don't think it is really correct and might cause issues.
We should make these relative and remove the / from the paths (here and in all apps where we have this). These routes are nested within the /insights/patch path and starting the path for them with a / is not correct.
We should also consistently use the InsightsLink and useInsightsNavigate, because these will turn relative paths like "advisories" into "/insights/patch/advisories".
These changes seem unnecessary and everything kinda works at the moment, but this is only because of trickery and luck. What these changes do is make our routing in apps comply with React router conventions in order to reduce the burden on routing.
I also saw some ".." as a to attribute of Link components, these are dangerous, because they can lead to different places under a different context, for example with federated modules.
There was a problem hiding this comment.
I think I get your point, this makes sense to use relative paths in accordance to the react-router documentation. Regarding the cut paths: I don't think it's just of trickery and luck :D IIRC, it's made on purpose that our applications have baseName set to / even though we always have some /insights/abc preceding. Which is made to avoid real base name rewrite or make the navigation work properly.
I will move the things to relative paths now.
Description
Associated Jira ticket: No jira.
This slightly improves the routes handling in Patch. Also removing ErrorComponent prop passing from ZeroState since it's not simply supported.
How to test the PR
No functionality should change. The routes must work correctly.
Before the change
The prior routes setup is more error prone or has unnecessary layers.
After the change
Life should be... uhm... better.
Dependent work link
Checklist: