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
Open

Update: React router dom from v5 to v6 #2580

Show file tree
Hide file tree
Changes from 12 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",
Zen-cronic marked this conversation as resolved.
Show resolved Hide resolved
"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
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 history={history}>
<TraceIDSearchInput />
</Router>
</BrowserRouter>
</HistoryProvider>
);
});
Expand Down
176 changes: 59 additions & 117 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 @@ -52,124 +52,66 @@ exports[`JaegerUIApp does not explode 1`] = `
}
}
>
<HistoryProvider
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],
}
}
>
<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))>
<Routes>
<Route
element={<WithRouteProps />}
path="/search"
/>
<Route
element={<WithRouteProps />}
path="/trace/: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"
/>
}
path="/"
/>
<Route
element={
<Navigate
to="/search"
/>
}
path=""
/>
<Route
element={
<Navigate
to="/search"
/>
}
}
>
<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
to="/search"
/>
</Route>
<Route>
<NotFound />
</Route>
</Switch>
</Memo(Connect(WithRouteProps))>
</Router>
</HistoryProvider>
path="/"
/>
<Route
element={<NotFound />}
path="*"
/>
</Routes>
</Memo(Connect(WithRouteProps))>
</Provider>
</ConfigProvider>
`;
63 changes: 18 additions & 45 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 All @@ -30,7 +30,7 @@ import SearchTracePage from '../SearchTracePage';
import { ROUTE_PATH as searchPath } from '../SearchTracePage/url';
import TraceDiff from '../TraceDiff';
import { ROUTE_PATH as traceDiffPath } from '../TraceDiff/url';
import TracePage from '../TracePage';
import TracePage from '../TracePage/index';
Zen-cronic marked this conversation as resolved.
Show resolved Hide resolved
import { ROUTE_PATH as tracePath } from '../TracePage/url';
import MonitorATMPage from '../Monitor';
import { ROUTE_PATH as monitorATMPath } from '../Monitor/url';
Expand All @@ -42,8 +42,7 @@ import '../common/vars.css';
import '../common/utils.css';
import 'antd/dist/reset.css';
import './index.css';
import { history, store } from '../../utils/configure-store';
import { HistoryProvider } from '../../utils/useHistory';
Copy link
Member

Choose a reason for hiding this comment

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

can this module be deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not in this PR, other unrelated tests depend on it. We should remove history on its own PR

import { store } from '../../utils/configure-store';

const jaegerTheme = {
token: {
Expand Down Expand Up @@ -88,49 +87,23 @@ export default class JaegerUIApp extends Component {
return (
<ConfigProvider theme={jaegerTheme}>
<Provider store={store}>
<HistoryProvider history={history}>
<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>
<Page>
<Routes>
<Route path={searchPath} element={<SearchTracePage />} />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

should use relative paths in v7 migration: e.g., search instead of /search

<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 path="/" element={<Navigate to={searchPath} />} />
<Route path={prefixUrl()} element={<Navigate to={searchPath} />} />
<Route path={prefixUrl('/')} element={<Navigate to={searchPath} />} />

<Route>
<NotFound />
</Route>
</Switch>
</Page>
</Router>
</HistoryProvider>
<Route path="*" element={<NotFound />} />
</Routes>
</Page>
</Provider>
</ConfigProvider>
);
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
4 changes: 1 addition & 3 deletions packages/jaeger-ui/src/components/DeepDependencies/url.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,8 @@ import prefixUrl from '../../utils/prefix-url';

export const ROUTE_PATH = prefixUrl('/deep-dependencies');

const ROUTE_MATCHER = { path: ROUTE_PATH, strict: true, exact: true };

export function matches(path: string) {
return Boolean(matchPath(path, ROUTE_MATCHER));
return Boolean(matchPath(path, ROUTE_PATH));
}

export function getUrl(args?: { [key: string]: unknown; showOp?: boolean }, baseUrl: string = ROUTE_PATH) {
Expand Down
4 changes: 1 addition & 3 deletions packages/jaeger-ui/src/components/DependencyGraph/url.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ import prefixUrl from '../../utils/prefix-url';

export const ROUTE_PATH = prefixUrl('/dependencies');

const ROUTE_MATCHER = { path: ROUTE_PATH, strict: true, exact: true };

export function matches(path: string) {
return Boolean(matchPath(path, ROUTE_MATCHER));
return Boolean(matchPath(path, ROUTE_PATH));
}

export function getUrl() {
Expand Down
4 changes: 1 addition & 3 deletions packages/jaeger-ui/src/components/Monitor/url.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,8 @@ import prefixUrl from '../../utils/prefix-url';

export const ROUTE_PATH = prefixUrl('/monitor');

const ROUTE_MATCHER = { path: ROUTE_PATH, strict: true, exact: true };

export function matches(path: string) {
return Boolean(matchPath(path, ROUTE_MATCHER));
return Boolean(matchPath(path, ROUTE_PATH));
}

export function getUrl() {
Expand Down
Loading
Loading