Skip to content
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

Merged
merged 4 commits into from
Jul 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
478 changes: 205 additions & 273 deletions web/package-lock.json

Large diffs are not rendered by default.

16 changes: 8 additions & 8 deletions web/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -32,26 +32,27 @@
"@babel/preset-env": "^7.17.12",
"@babel/preset-react": "^7.17.12",
"@babel/preset-typescript": "^7.17.12",
"@openshift-console/dynamic-plugin-sdk": "0.0.17",
"@openshift-console/dynamic-plugin-sdk-webpack": "0.0.8",
"@openshift-console/dynamic-plugin-sdk": "0.0.19",
"@openshift-console/dynamic-plugin-sdk-webpack": "0.0.10",
"@testing-library/react": "^12.1.2",
"@types/copy-webpack-plugin": "8.0.1",
"@types/enzyme": "3.10.x",
"@types/jest": "27.0.3",
"@types/jsdom": "^16.2.13",
"@types/lodash": "^4.14.178",
"@types/murmurhash-js": "^1.0.3",
"@types/port-numbers": "^5.0.0",
"@types/react": "^17.0.1",
"@types/react-dom": "^17.0.1",
"@types/react-modal": "^3.13.1",
"@types/react-router-dom": "^5.2.0",
"@types/react-router": "5.1.20",
"@types/react-router-dom": "^5.3.3",
"@types/react-transition-group": "^4.4.4",
"@types/webpack-dev-server": "^4.5.0",
"@typescript-eslint/eslint-plugin": "5.45.0",
"@typescript-eslint/parser": "5.45.0",
"@typescript-eslint/typescript-estree": "5.45.0",
"@wojtekmaj/enzyme-adapter-react-17": "^0.6.6",
"axios": "^0.24.0",
"babel-loader": "^8.2.5",
"babel-preset-react": "^6.24.1",
"copy-webpack-plugin": "6.4.1",
Expand Down Expand Up @@ -105,8 +106,7 @@
"@patternfly/react-core": "4.276.6",
"@patternfly/react-table": "4.112.39",
"@patternfly/react-topology": "4.91.38",
"@types/murmurhash-js": "^1.0.3",
"history": "^5.1.0",
"axios": "^0.24.0",
"html-to-image": "^1.11.11",
"i18next": "^19.8.3",
"i18next-http-backend": "^1.0.21",
Expand All @@ -117,8 +117,8 @@
"react-dom": "^17.0.2",
"react-i18next": "^11.8.11",
"react-modal": "^3.14.4",
"react-router": "^5.2.0",
"react-router-dom": "^5.2.0"
"react-router": "^6.14.0",
"react-router-dom": "^6.14.0"
},
"overrides": {
"glob-parent": "^6.0.1"
Expand Down
2 changes: 2 additions & 0 deletions web/setup-tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,8 @@ jest.mock('react-router-dom', () => ({
useHistory: () => ({
push: jest.fn(),
}),
// navigate replace history in v6
useNavigate: () => jest.fn(),
Link: () => {
return null;
}
Expand Down
1 change: 1 addition & 0 deletions web/src/api/ipfix.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
/* eslint-disable max-len */
import { RecordType } from '../model/flow-query';

// Please keep this file documented: it is used in doc generation
Expand Down
24 changes: 9 additions & 15 deletions web/src/app.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,11 @@ import {
PageHeaderTools,
PageHeaderToolsGroup,
PageHeaderToolsItem,
PageSection,
PageSidebar,
Radio
} from '@patternfly/react-core';
import React from 'react';
import { BrowserRouter as Router, Link, Route } from 'react-router-dom';
import { BrowserRouter, Link, Route, Routes } from 'react-router-dom';
import NetflowTrafficParent from './components/netflow-traffic-parent';

interface AppState {
Expand Down Expand Up @@ -61,14 +60,11 @@ export class App extends React.Component<{}, AppState> {
};

private getPages = () => (
<React.Fragment>
<Routes>
{pages.map(page => (
<Route
render={() => <PageSection style={{ zIndex: 2 }}>{this.getPageContent(page.id)}</PageSection>}
key={page.id}
/>
<Route element={this.getPageContent(page.id)} path={page.id} key={page.id} />
))}
</React.Fragment>
</Routes>
);

render() {
Expand Down Expand Up @@ -130,13 +126,11 @@ export class App extends React.Component<{}, AppState> {
const AppSidebar = <PageSidebar isNavOpen={isNavOpen} nav={nav} />;

return (
<>
<Router>
<Page header={AppHeader} sidebar={AppSidebar} isManagedSidebar>
{this.getPages()}
</Page>
</Router>
</>
<BrowserRouter>
<Page header={AppHeader} sidebar={AppSidebar} isManagedSidebar>
{this.getPages()}
</Page>
</BrowserRouter>
);
}
}
Expand Down
7 changes: 3 additions & 4 deletions web/src/components/alerts/banner.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,22 +10,21 @@ import {
} from '@patternfly/react-core';
import './banner.css';
import { Rule } from '@openshift-console/dynamic-plugin-sdk';
import { useHistory } from 'react-router-dom';
import { navigate } from '../dynamic-loader/dynamic-loader';

export const AlertBanner: React.FC<{
rule: Rule;
onDelete: () => void;
}> = ({ rule, onDelete }) => {
const history = useHistory();
const { t } = useTranslation('plugin__netobserv-plugin');
const routeAlert = () => {
let path = `/monitoring/alerts/${rule.id}`;
path += `?alertname=${rule.name}&app=${rule.labels.app}&severity=${rule.labels.severity}`;
history.push(path);
navigate(path);
};
const routeDashboard = () => {
const path = `/monitoring/dashboards/grafana-dashboard-netobserv-health`;
history.push(path);
navigate(path);
};
return (
<div className="netobserv-alert">
Expand Down
39 changes: 39 additions & 0 deletions web/src/components/dynamic-loader/dynamic-loader.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
/* eslint-disable @typescript-eslint/no-explicit-any */
import * as React from 'react';
import * as reactRouterDom from 'react-router-dom';

/**
* useHistory is deprecated and not available anymore in react-router 6+
* This function allow retro compatibility between useHistory and useNavigate
* by switching from one to the other
*/
type NavFunc = (to: string, opts?: any) => void;
let navFunc: NavFunc | null = null;

export const setNavFunction = (f: NavFunc) => {
navFunc = f;
};

export const navigate = (to: string, opts?: any) => {
if (navFunc) {
navFunc(to, opts);
} else {
console.error('navigate error; navFunc is not initialized', navFunc);
}
};

export const DynamicLoader: React.FC<{}> = ({ children }) => {
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");
}
Comment on lines +26 to +35
Copy link
Contributor Author

@jpinsonneau jpinsonneau Jun 29, 2023

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

Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

return <>{!!children ? children : ''}</>;
};

export default DynamicLoader;
7 changes: 3 additions & 4 deletions web/src/components/filters/filters-toolbar.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import { TimesIcon, TimesCircleIcon } from '@patternfly/react-icons';
import * as _ from 'lodash';
import * as React from 'react';
import { useTranslation } from 'react-i18next';
import { useHistory } from 'react-router-dom';
import {
Filter,
FilterComponent,
Expand All @@ -42,6 +41,7 @@ import { getFilterFullName, Indicator } from './filters-helper';
import TextFilter from './text-filter';
import { LOCAL_STORAGE_SHOW_FILTERS_KEY, useLocalStorage } from '../../utils/local-storage-hook';
import './filters-toolbar.css';
import { navigate } from '../dynamic-loader/dynamic-loader';
import CompareFilter, { FilterCompare } from './compare-filter';

export interface FiltersToolbarProps {
Expand Down Expand Up @@ -71,7 +71,6 @@ export const FiltersToolbar: React.FC<FiltersToolbarProps> = ({
allowConnectionFilter,
...props
}) => {
const { push } = useHistory();
const { t } = useTranslation('plugin__netobserv-plugin');
const [indicator, setIndicator] = React.useState<Indicator>(ValidatedOptions.default);
const [message, setMessage] = React.useState<string | undefined>();
Expand Down Expand Up @@ -225,7 +224,7 @@ export const FiltersToolbar: React.FC<FiltersToolbarProps> = ({
<Button
id="edit-filters-button"
data-test="edit-filters-button"
onClick={() => push(getPathWithParams(netflowTrafficPath))}
onClick={() => navigate(getPathWithParams(netflowTrafficPath))}
>
{t('Edit filters')}
</Button>
Expand All @@ -247,7 +246,7 @@ export const FiltersToolbar: React.FC<FiltersToolbarProps> = ({
)}
</ToolbarGroup>
);
}, [clearFilters, filters, forcedFilters, defaultFilters, push, setFilters, resetFilters, t]);
}, [clearFilters, filters, forcedFilters, defaultFilters, setFilters, resetFilters, t]);

const countActiveFilters = (forcedFilters || filters || []).reduce((prev, cur) => prev + cur.values.length, 0);
let showHideText: string | undefined;
Expand Down
18 changes: 10 additions & 8 deletions web/src/components/messages/loki-error.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import { Link } from 'react-router-dom';
import { getBuildInfo, getLimits, getMetrics, getLokiReady } from '../../api/routes';
import { getHTTPErrorDetails } from '../../utils/errors';
import './loki-error.css';
import { useHistory } from 'react-router-dom';

export type Size = 's' | 'm' | 'l';

Expand All @@ -43,7 +42,6 @@ export const LokiError: React.FC<Props> = ({ title, error }) => {
const [ready, setReady] = React.useState<string | undefined>();
const [infoName, setInfoName] = React.useState<string | undefined>();
const [info, setInfo] = React.useState<string | undefined>();
const history = useHistory();

const updateInfo = React.useCallback(
(type: LokiInfo) => {
Expand Down Expand Up @@ -164,12 +162,10 @@ export const LokiError: React.FC<Props> = ({ title, error }) => {
</Text>
{error.includes('too many outstanding requests') && (
<Text component={TextVariants.blockquote}>
{
{t(
// eslint-disable-next-line max-len
t(
'Ensure Loki config contains "parallelise_shardable_queries: true" and "max_outstanding_requests_per_tenant: 2048"'
)
}
'Ensure Loki config contains "parallelise_shardable_queries: true" and "max_outstanding_requests_per_tenant: 2048"'
)}
</Text>
)}
</>
Expand Down Expand Up @@ -208,8 +204,14 @@ export const LokiError: React.FC<Props> = ({ title, error }) => {
{t('Show configuration limits')}
</Button>
<Button
onClick={() => history.push('/monitoring/dashboards/grafana-dashboard-netobserv-health')}
variant="link"
component={(props: React.FunctionComponent) => (
<Link
{...props}
target="_blank"
to={{ pathname: '/monitoring/dashboards/grafana-dashboard-netobserv-health' }}
/>
)}
>
{t('Show health dashboard')}
</Button>
Expand Down
1 change: 1 addition & 0 deletions web/src/components/netflow-overview/panel-kebab.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ export const PanelKebab: React.FC<PanelKebabProps> = ({ id, options, setOptions,
content={
<Text component={TextVariants.p}>
{t(
// eslint-disable-next-line max-len
'Show scope-internal traffic, depending on the selected scope (e.g. node-internal traffic, namespace-internal traffic)'
)}
</Text>
Expand Down
9 changes: 6 additions & 3 deletions web/src/components/netflow-traffic-parent.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { clearURLParams } from '../utils/url';
import { clearLocalStorage } from '../utils/local-storage-hook';
import NetflowTraffic from './netflow-traffic';
import AlertFetcher from './alerts/fetcher';
import DynamicLoader from './dynamic-loader/dynamic-loader';

type Props = {};
type State = {
Expand Down Expand Up @@ -52,9 +53,11 @@ class NetflowTrafficParent extends React.Component<Props, State> {
// else render default NetworkTraffic

return (
<AlertFetcher>
<NetflowTraffic forcedFilters={null} />
</AlertFetcher>
<DynamicLoader>
<AlertFetcher>
<NetflowTraffic forcedFilters={null} />
</AlertFetcher>
</DynamicLoader>
Comment on lines +56 to +60
Copy link
Contributor Author

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.

);
}
}
Expand Down
25 changes: 12 additions & 13 deletions web/src/components/netflow-traffic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ import { ColumnsIcon, CompressIcon, EllipsisVIcon, ExpandIcon, ExportIcon, SyncA
import * as _ from 'lodash';
import * as React from 'react';
import { useTranslation } from 'react-i18next';
import { useHistory } from 'react-router-dom';
import { useTheme } from '../utils/theme-hook';
import { Record } from '../api/ipfix';
import { RecordsResult, Stats, TopologyMetrics, TopologyResult } from '../api/loki';
Expand Down Expand Up @@ -139,14 +138,14 @@ import { formatDuration, getDateMsInSeconds, getDateSInMiliseconds, parseDuratio
import GuidedTourPopover, { GuidedTourHandle } from './guided-tour/guided-tour';

import { exportToPng } from '../utils/export';
import { navigate } from './dynamic-loader/dynamic-loader';

export type ViewId = 'overview' | 'table' | 'topology';

export const NetflowTraffic: React.FC<{
forcedFilters?: Filter[] | null;
isTab?: boolean;
}> = ({ forcedFilters, isTab }) => {
const { push } = useHistory();
const { t } = useTranslation('plugin__netobserv-plugin');
const [extensions] = useResolvedExtensions<ModelFeatureFlag>(isModelFeatureFlag);
const k8sModels = useK8sModelsWithColors();
Expand Down Expand Up @@ -535,30 +534,30 @@ export const NetflowTraffic: React.FC<{
React.useEffect(() => {
//with forced filters in url if specified
if (forcedFilters) {
setURLFilters(forcedFilters!);
setURLFilters(forcedFilters!, true);
} else if (!_.isEmpty(filters)) {
//write filters in url if not empty
setURLFilters(filters);
setURLFilters(filters, !initState.current.includes('configLoaded'));
}
}, [filters, forcedFilters]);
React.useEffect(() => {
setURLRange(range);
setURLRange(range, !initState.current.includes('configLoaded'));
}, [range]);
React.useEffect(() => {
setURLLimit(limit);
setURLLimit(limit, !initState.current.includes('configLoaded'));
}, [limit]);
React.useEffect(() => {
setURLMatch(match);
setURLMatch(match, !initState.current.includes('configLoaded'));
}, [match]);
React.useEffect(() => {
setURLReporter(reporter);
setURLReporter(reporter, !initState.current.includes('configLoaded'));
}, [reporter]);
React.useEffect(() => {
setURLMetricFunction(metricFunction);
setURLMetricType(metricType);
setURLMetricFunction(metricFunction, !initState.current.includes('configLoaded'));
setURLMetricType(metricType, !initState.current.includes('configLoaded'));
}, [metricFunction, metricType]);
React.useEffect(() => {
setURLRecortType(recordType);
setURLRecortType(recordType, !initState.current.includes('configLoaded'));
}, [recordType]);

// update local storage saved query params
Expand Down Expand Up @@ -593,13 +592,13 @@ export const NetflowTraffic: React.FC<{

const clearFilters = React.useCallback(() => {
if (forcedFilters) {
push(netflowTrafficPath);
navigate(netflowTrafficPath);
} else if (filters) {
//set URL Param to empty value to be able to restore state coming from another page
setURLFilters([]);
updateTableFilters([]);
}
}, [forcedFilters, push, filters, updateTableFilters]);
}, [forcedFilters, filters, updateTableFilters]);

const viewTabs = () => {
return (
Expand Down
Loading
Loading