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.

Sign up for GitHub

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

Update: React router dom from v5 to v6 #2580

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
+183 −373
Open

Update: React router dom from v5 to v6 #2580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
2af7813
Update: remove Redirect inside Switch
Zen-cronic Jan 10, 2025
154c7c0
Update: install react-router-dom v6 latest
Zen-cronic Jan 10, 2025
2455d90
Reinstall @types/history as some app modules depend on it
Zen-cronic Jan 10, 2025
8a40827
Revert "Update: install react-router-dom v6 latest"
Zen-cronic Jan 10, 2025
2743cca
Replace Route with CompatRoute for incremental migration
Zen-cronic Jan 10, 2025
3ee6445
Update: Replace react-router-dom imports with compat imports
Zen-cronic Jan 13, 2025
828e985
Merge branch 'main' into update/react-router-dom-v7
Zen-cronic Jan 13, 2025
d5eb985
Update: Replace compat imports with react-router-dom-v6 imports
Zen-cronic Jan 13, 2025
93c2181
Fix: Use `useLocation` hook in functional TopNav component
Zen-cronic Jan 13, 2025
52793ea
Fix: Snapshot for rrd-v6
Zen-cronic Jan 13, 2025
aa98700
Fix: Lint tsc
Zen-cronic Jan 13, 2025
c1eaadd
Update: Remove `@types/react-router-dom`
Zen-cronic Jan 13, 2025
188ae81
Update: Applied review changes
Zen-cronic Jan 13, 2025
8aebf4c
Update: Use `Outlet` in `Page`
Zen-cronic Jan 13, 2025
375bed2
Update: Revert BrowserRouter usage in test
Zen-cronic Jan 14, 2025
9e691d6
Fix: Update regex route path matching to optional parameters
Zen-cronic Jan 14, 2025
4228454
Fix: Update dep version to match lock file
Zen-cronic Jan 14, 2025
b0ac58a
Fix: Display Deep Dependency Graph on click
Zen-cronic Jan 16, 2025
fe98dc1
Fix: Lint
Zen-cronic Jan 16, 2025
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
191 changes: 35 additions & 156 deletions package-lock.json
View file
Open in desktop

Large diffs are not rendered by default.

7 changes: 3 additions & 4 deletions packages/jaeger-ui/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,11 @@
"@testing-library/jest-dom": "^6.4.5",
"@testing-library/react": "^15.0.7",
"@types/deep-freeze": "^0.1.1",
"@types/history": "^4.7.11",
"@types/lodash": "^4.17.0",
"@types/object-hash": "^3.0.2",
"@types/react": "^18.3.11",
"@types/react-helmet": "^6.1.5",
"@types/react-router-dom": "^5.1.0",
"@types/react-window": "^1.8.0",
"@types/redux-actions": "2.2.1",
"@vitejs/plugin-legacy": "^6.0.0",
Expand All @@ -48,7 +48,7 @@
"@ant-design/compatible": "^5.1.3",
"@jaegertracing/plexus": "0.2.0",
"@pyroscope/flamegraph": "0.21.4",
"@sentry/browser": "^8.18.0",
"@sentry/browser": "^8.48.0",
"antd": "^5.21.3",
"chance": "^1.0.10",
"classnames": "^2.5.1",
Expand Down Expand Up @@ -76,8 +76,7 @@
"react-is": "^18.2.0",
"react-json-view-lite": "2.1.0",
"react-redux": "^8.1.2",
"react-router-dom": "5.3.4",
"react-router-dom-v5-compat": "^6.24.0",
"react-router-dom": "^6.28.1",
"react-vis": "1.11.12",
"react-vis-force": "^0.3.1",
"react-window": "^1.8.10",
Expand Down
5 changes: 4 additions & 1 deletion packages/jaeger-ui/src/components/App/Page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { Layout } from 'antd';
import cx from 'classnames';
import Helmet from 'react-helmet';
import { connect } from 'react-redux';
import { Outlet } from 'react-router-dom';

import TopNav from './TopNav';
import { ReduxState } from '../../types';
Expand Down Expand Up @@ -62,7 +63,9 @@ export class PageImpl extends React.Component<TProps> {
<TopNav />
</Header>
)}
<Content className={contentCls}>{this.props.children}</Content>
<Content className={contentCls}>
<Outlet />
</Content>
</Layout>
</div>
);
Expand Down
7 changes: 4 additions & 3 deletions packages/jaeger-ui/src/components/App/TopNav.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import { Dropdown, Menu, MenuProps } from 'antd';
import { IoChevronDown } from 'react-icons/io5';
import _has from 'lodash/has';
import { connect } from 'react-redux';
import { Link } from 'react-router-dom';
import { Link, useLocation } from 'react-router-dom';

import TraceIDSearchInput from './TraceIDSearchInput';
import * as dependencyGraph from '../DependencyGraph/url';
Expand Down Expand Up @@ -119,8 +119,9 @@ const itemsGlobalLeft: MenuProps['items'] = [
];

export function TopNavImpl(props: Props) {
const { config, router } = props;
const { pathname } = router.location;
const location = useLocation();
const { config } = props;
const { pathname } = location;
const menuItems = Array.isArray(config.menu) ? config.menu : [];

const itemsGlobalRight: MenuProps['items'] = [
Expand Down
6 changes: 3 additions & 3 deletions packages/jaeger-ui/src/components/App/TraceIDSearchInput.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ import React from 'react';
import { createMemoryHistory } from 'history';
import { render, screen, fireEvent } from '@testing-library/react';
import '@testing-library/jest-dom';
import { Router } from 'react-router-dom';
import { BrowserRouter } from 'react-router-dom';
import TraceIDSearchInput from './TraceIDSearchInput';
import { HistoryProvider } from '../../utils/useHistory';

Expand All @@ -29,9 +29,9 @@ describe('<TraceIDSearchInput />', () => {
history = createMemoryHistory();
render(
<HistoryProvider history={history}>
Copy link
Member

Choose a reason for hiding this comment

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

why in packages/jaeger-ui/src/components/App/index.jsx the history provider is removed but here it's not? What is the motivation for either decision?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In packages/jaeger-ui/src/components/App/index.jsx, HistoryProvider is removed b/c rrd v6 works without being wrapped with that component.

But in this test and other tests, the history object from it's namesake package is used for assertions. So, removing HistoryProvider would mean a rewrite for these tests.

Copy link
Member

Choose a reason for hiding this comment

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

but what's the story with the history package, aren't we going to have to remove it altogether as incompatible going forward? It's ok if that clean-up can be done incrementally, we can defer it to other PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

history is no longer a peer dep in v6 and later. And yes, we should have a separate PR for removing it from this repo.

And we need to consider how to replace the function of that package - currently, it's used to provide the navigation props (e.g., history.push(/)) in class-based components wrapped with withRouteProps. Because they're class based, we can't use hooks like useNavigation). This discussion is better continued in another PR :)

Copy link
Member

Choose a reason for hiding this comment

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

Specifically, the migration guide you linked recommends using navigate instead of history.

Copy link
Member

Choose a reason for hiding this comment

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

And the docs say BrowserRouter incorporates the history in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Specifically, the migration guide you linked recommends using navigate instead of history.

yep, we should replace history with navigate from the useNavigate hook. But that change can't be made unless the class-based components (like this) are rewritten into functional components.

<Router history={history}>
<BrowserRouter>
<TraceIDSearchInput />
</Router>
</BrowserRouter>
</HistoryProvider>
);
});
Expand Down
139 changes: 49 additions & 90 deletions packages/jaeger-ui/src/components/App/__snapshots__/index.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,98 +77,57 @@ exports[`JaegerUIApp does not explode 1`] = `
}
}
>
<Router
history={
Object {
"action": "POP",
"back": [Function],
"block": [Function],
"createHref": [Function],
"forward": [Function],
"go": [Function],
"goBack": [Function],
"goForward": [Function],
"length": 1,
"listen": [Function],
"listenObject": false,
"location": Object {
"hash": "",
"pathname": "/",
"search": "",
"state": undefined,
},
"push": [Function],
"replace": [Function],
}
}
>
<Memo(Connect(WithRouteProps))>
<Switch>
<Route
path="/search"
>
<WithRouteProps />
</Route>
<Route
path="/trace/:a?\\\\.\\\\.\\\\.:b?"
>
<WithRouteProps />
</Route>
<Route
path="/trace/:id"
>
<WithRouteProps />
</Route>
<Route
path="/dependencies"
>
<WithRouteProps />
</Route>
<Route
path="/deep-dependencies"
>
<WithRouteProps />
</Route>
<Route
path="/quality-metrics"
>
<WithRouteProps />
</Route>
<Route
path="/monitor"
>
<MonitorATMPage />
</Route>
<Route
exact={true}
path="/"
>
<Redirect
to="/search"
/>
</Route>
<Route
exact={true}
path=""
>
<Redirect
to="/search"
/>
</Route>
<Route
exact={true}
path="/"
>
<Redirect
<Routes>
<Route
element={<Memo(Connect(WithRouteProps)) />}
path="/"
>
<Route
element={<WithRouteProps />}
index={true}
/>
<Route
element={<WithRouteProps />}
path="/search"
/>
<Route
element={<WithRouteProps />}
path="/trace-diff/:a?/:b?"
/>
<Route
element={<WithRouteProps />}
path="/trace/:id"
/>
<Route
element={<WithRouteProps />}
path="/dependencies"
/>
<Route
element={<WithRouteProps />}
path="/deep-dependencies"
/>
<Route
element={<WithRouteProps />}
path="/quality-metrics"
/>
<Route
element={<MonitorATMPage />}
path="/monitor"
/>
<Route
element={
<Navigate
to="/search"
/>
</Route>
<Route>
<NotFound />
</Route>
</Switch>
</Memo(Connect(WithRouteProps))>
</Router>
}
path=""
/>
<Route
element={<NotFound />}
path="*"
/>
</Route>
</Routes>
</HistoryProvider>
</Provider>
</ConfigProvider>
Expand Down
56 changes: 15 additions & 41 deletions packages/jaeger-ui/src/components/App/index.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

import React, { Component } from 'react';
import { Provider } from 'react-redux';
import { Route, Redirect, Switch, Router } from 'react-router-dom';
import { Route, Routes, Navigate } from 'react-router-dom';

import { ConfigProvider } from 'antd';
import { defaultTheme } from '@ant-design/compatible';
Expand Down Expand Up @@ -89,47 +89,21 @@ export default class JaegerUIApp extends Component {
<ConfigProvider theme={jaegerTheme}>
<Provider store={store}>
<HistoryProvider history={history}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

update: HistoryProvider is still needed by the router b/c history is used for navigation:

jaeger-ui/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.tsx

Line 116 in 1ee19c2

history.push(getUrl({ ...urlState, view }));

useNavigate can't be used inside a class-based component.

<Router history={history}>
<Page>
<Switch>
<Route path={searchPath}>
<SearchTracePage />
</Route>
<Route path={traceDiffPath}>
<TraceDiff />
</Route>
<Route path={tracePath}>
<TracePage />
</Route>
<Route path={dependenciesPath}>
<DependencyGraph />
</Route>
<Route path={deepDependenciesPath}>
<DeepDependencies />
</Route>
<Route path={qualityMetricsPath}>
<QualityMetrics />
</Route>
<Route path={monitorATMPath}>
<MonitorATMPage />
</Route>
<Routes>
<Route path="/" element={<Page />}>
<Route index element={<SearchTracePage />} />
<Route path={searchPath} element={<SearchTracePage />} />
<Route path={traceDiffPath} element={<TraceDiff />} />
<Route path={tracePath} element={<TracePage />} />
<Route path={dependenciesPath} element={<DependencyGraph />} />
<Route path={deepDependenciesPath} element={<DeepDependencies />} />
<Route path={qualityMetricsPath} element={<QualityMetrics />} />
<Route path={monitorATMPath} element={<MonitorATMPage />} />

<Route exact path="/">
<Redirect to={searchPath} />
</Route>
<Route exact path={prefixUrl()}>
<Redirect to={searchPath} />
</Route>
<Route exact path={prefixUrl('/')}>
<Redirect to={searchPath} />
</Route>

<Route>
<NotFound />
</Route>
</Switch>
</Page>
</Router>
<Route path={prefixUrl()} element={<Navigate to={searchPath} />} />
<Route path="*" element={<NotFound />} />
</Route>
</Routes>
</HistoryProvider>
</Provider>
</ConfigProvider>
Expand Down
6 changes: 5 additions & 1 deletion packages/jaeger-ui/src/components/DeepDependencies/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export type TOwnProps = {
history: RouterHistory;
location: Location;
showSvcOpsHeader: boolean;
urlQueryParams: Record<string, string | null>;
};

export type TProps = TDispatchProps & TReduxProps & TOwnProps;
Expand Down Expand Up @@ -411,7 +412,10 @@ export class DeepDependencyGraphPageImpl extends React.PureComponent<TProps, TSt
export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxProps {
const { services: stServices } = state;
const { services, serverOpsForService } = stServices;
const urlState = getUrlState(ownProps.location.search);

const locationUrlState = getUrlState(ownProps.location.search);
const urlState = { ...locationUrlState, ...ownProps.extraUrlArgs };

const { density, operation, service, showOp: urlStateShowOp } = urlState;
const showOp = urlStateShowOp !== undefined ? urlStateShowOp : operation !== undefined;
let graphState: TDdgStateEntry | undefined;
Expand Down
6 changes: 4 additions & 2 deletions packages/jaeger-ui/src/components/DeepDependencies/traces.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,9 @@ describe('TracesDdg', () => {
);
const search = queryString.stringify({ ...extraUrlArgs, extraParam: 'extraParam' });

const wrapper = shallow(<TracesDdgImpl location={{ search }} {...passProps} />);
const wrapper = shallow(
<TracesDdgImpl location={{ search }} {...passProps} navigateTo={`/search?${search}`} />
);
const ddgPage = wrapper.find(DeepDependencyGraphPageImpl);
expect(ddgPage.props()).toEqual(
expect.objectContaining({
Expand Down Expand Up @@ -145,7 +147,7 @@ describe('TracesDdg', () => {
mapStateToProps(state, ownProps);
spies.forEach(spy => {
const [call0, call1] = spy.mock.calls;
call0.forEach((arg, i) => expect(call1[i]).toBe(arg));
call0.forEach((arg, i) => expect(call1[i]).toStrictEqual(arg));
});
});

Expand Down
12 changes: 9 additions & 3 deletions packages/jaeger-ui/src/components/DeepDependencies/traces.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ const svcOp = memoizeOne((service, operation) => ({ service, operation }));

// export for tests
export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxProps {
const urlState = getUrlState(ownProps.location.search);
const locationUrlState = getUrlState(ownProps.location.search);
const urlState = { ...locationUrlState, ...ownProps.urlQueryParams };
Copy link
Contributor Author

@Zen-cronic Zen-cronic Jan 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

merging with the passed urlQueryParams b/c location and the redux state router are out of sync.

caveat of this approach: we need to do this to every component that are connected to the store and that affect the url in some way


const { density, operation, service, showOp: urlStateShowOp } = urlState;
const showOp = urlStateShowOp !== undefined ? urlStateShowOp : operation !== undefined;
let graphState: TDdgStateEntry | undefined;
Expand All @@ -63,15 +65,19 @@ export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxP
type TOwnProps = {
history: RouterHistory;
location: Location;
urlQueryParams: Record<string, string | null>;
navigateTo: string;
};

// export for tests
export class TracesDdgImpl extends React.PureComponent<TOwnProps & TReduxProps> {
render(): React.ReactNode {
const { location } = this.props;
const urlArgs = queryString.parse(location.search);
const { navigateTo } = this.props;

const urlArgs = queryString.parse(navigateTo.split('/search?')[1]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO fix:
parsing the manually passed url prop (that contains the correct params) like this is very hacky.

const { end, start, limit, lookback, maxDuration, minDuration, view } = urlArgs;
const extraArgs = { end, start, limit, lookback, maxDuration, minDuration, view };

return (
<DeepDependencyGraphPageImpl
baseUrl={ROUTE_PATH}
Expand Down
6 changes: 1 addition & 5 deletions packages/jaeger-ui/src/components/DeepDependencies/url.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,7 @@ describe('DeepDependencyGraph/url', () => {

it('calls matchPath with expected arguments', () => {
matches(path);
expect(matchPathSpy).toHaveBeenLastCalledWith(path, {
path: ROUTE_PATH,
strict: true,
exact: true,
});
expect(matchPathSpy).toHaveBeenLastCalledWith(path, ROUTE_PATH);
});

it("returns truthiness of matchPath's return value", () => {
Expand Down
Loading
Loading