-
Notifications
You must be signed in to change notification settings - Fork 14
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
NETOBSERV-1097 react-router-dom upgrade with dynamic loader #350
NETOBSERV-1097 react-router-dom upgrade with dynamic loader #350
Conversation
const genericReactRouterDom = reactRouterDom as any; | ||
if (genericReactRouterDom['useNavigate']) { | ||
console.log('loading nav function from react-router useNavigate'); | ||
setNavFunction(genericReactRouterDom['useNavigate']()); | ||
} else if (genericReactRouterDom['useHistory']) { | ||
console.log('loading nav function from react-router useHistory'); | ||
setNavFunction(genericReactRouterDom['useHistory']().push); | ||
} else { | ||
console.error("can't load nav function from react-router"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useNavigate
is the preferred hook and will be taken from the import if available.
If not, reactRouterDom.useNavigate
equals 0
and it fallback on useHistory.push
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just tested on my 4.13 cluster and it uses useHistory
. I guess I should spin up a 4.14 to test useNavigate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useNavigate
will be used only when console plugin team will finish the migration introducing v6 API.
- prior to 4.14 we rely on v5 api
- ocp 4.14 will introduce
react-router-dom-v5-compat
with v6 capabilities using this compat lib. Since we don't import it it will still rely on v5 api - a later version will introduce v6 api, that's when we'll switch to
useNavigate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<DynamicLoader> | ||
<AlertFetcher> | ||
<NetflowTraffic forcedFilters={null} /> | ||
</AlertFetcher> | ||
</DynamicLoader> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DynamicLoader
FC is loaded on top of everything to ensure the hooks are up to date.
Codecov Report
@@ Coverage Diff @@
## main #350 +/- ##
==========================================
- Coverage 58.67% 57.99% -0.69%
==========================================
Files 150 152 +2
Lines 6619 6706 +87
Branches 800 804 +4
==========================================
+ Hits 3884 3889 +5
- Misses 2518 2595 +77
- Partials 217 222 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
|
}; | ||
|
||
export const setURLParam = (param: URLParam, value: string) => { | ||
export const setURLParam = (param: URLParam, value: string, replace?: boolean) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
previously with useHistory
, we did never replace the url, right? Is it necessary to do this distinction when using useNavigate
, can't we just have always replace=false?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do in fact
history.replace(`${url.pathname}?${params.toString()}${url.hash}`); |
The replace is usefull to avoid having a huge history at startup, when React hooks set url parameters one by one
network-observability-console-plugin/web/src/components/netflow-traffic.tsx
Lines 188 to 200 in eadc9be
// Rewrite URL params on state change | |
React.useEffect(() => { | |
setURLFilters(forcedFilters || filters); | |
}, [filters, forcedFilters]); | |
React.useEffect(() => { | |
setURLRange(range); | |
}, [range]); | |
React.useEffect(() => { | |
setURLLimit(limit); | |
}, [limit]); | |
React.useEffect(() => { | |
setURLMatch(match); | |
}, [match]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I see
Thanks @jpinsonneau ! |
@memodi I guess this one can be merged? There will be regression tests to do on all supported OCPs so I guess it can be part of the usual pre-release tests |
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jotak The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR introduce a dynamic loader component that allows plugin to switch between
useHistory
anduseNavigate
hooks provided byreact-router-dom
lib embedded in OCP console.It's a better approach than #348 since it works on any OCP version.
The downside of this approach is that we still rely on v5 API until OCP console move to v6 (no usage of
react-router-dom-v5-compat
lib)