Skip to content

Commit 72014ef

Browse files
authored
Phase 3.1: Replace state.router.location with useLocation() (#3574)
## Which problem is this PR solving? - Resolves part of #3313 ## Description of the changes - Components were reading the current URL from Redux like this: `state.router.location.search -> e.g. "?service=frontend&limit=20"` This means the URL was being stored in Redux and components were reading it from there. - The new way in this PR: components use a React Router hook: `const { search } = useLocation(); -> e.g. "?service=frontend&limit=20"` This means ask React Router directly what the current URL search is. No Redux needed. #### Why make this change? Because in our next phases, we will remove the `history` and `redux-first-history` packages. Those packages are what put the URL into Redux in the first place. So, before we can remove them, every component that reads from `state.router.location` must be updated to use hooks. #### Every changes in this PR using one of these three patterns: - Hook directly in component (used in `Page.tsx`, `UiFindInput.tsx`) - Wrapper component (used in `SearchForm.tsx`, `DdgNodeContent.tsx`, `DetailsPanel.tsx`) - Already-injected prop (used in `TraceDiff.tsx`) ## How was this change tested? - Unit tests - Components working properly ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully: `make lint test` ## AI Usage in this PR (choose one) See [AI Usage Policy](https://github.com/jaegertracing/jaeger/blob/main/CONTRIBUTING_GUIDELINES.md#ai-usage-policy). - [ ] **None**: No AI tools were used in creating this PR - [x] **Light**: AI provided minor assistance (formatting, simple suggestions) **[gave suggestions and fixing the failed unit tests, lint errors]** - [ ] **Moderate**: AI helped with code generation or debugging specific parts - [ ] **Heavy**: AI generated most or all of the code changes --------- Signed-off-by: Parship Chowdhury <parshipchowdhury@gmail.com>
1 parent 1b216f4 commit 72014ef

File tree

18 files changed

+287
-159
lines changed

18 files changed

+287
-159
lines changed

packages/jaeger-ui/src/components/App/Page.test.js

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -7,74 +7,82 @@ jest.mock('../../utils/tracking');
77
import React from 'react';
88
import { render, screen } from '@testing-library/react';
99
import '@testing-library/jest-dom';
10+
import { MemoryRouter } from 'react-router-dom-v5-compat';
1011

1112
import { mapStateToProps, PageImpl as Page } from './Page';
1213
import { trackPageView } from '../../utils/tracking';
1314

15+
const renderWithPath = (props, path = '/test?search=value') =>
16+
render(
17+
<MemoryRouter initialEntries={[path]}>
18+
<Page {...props} />
19+
</MemoryRouter>
20+
);
21+
1422
describe('mapStateToProps()', () => {
15-
it('maps state to props', () => {
16-
const pathname = 'a-pathname';
17-
const search = 'a-search';
18-
const state = {
19-
router: { location: { pathname, search } },
20-
};
21-
expect(mapStateToProps(state)).toEqual({ pathname, search });
23+
it('maps embedded state to props', () => {
24+
const state = { embedded: true };
25+
expect(mapStateToProps(state)).toEqual({ embedded: true });
26+
});
27+
28+
it('does not include pathname or search (now from useLocation)', () => {
29+
const result = mapStateToProps({ embedded: false });
30+
expect(result).not.toHaveProperty('pathname');
31+
expect(result).not.toHaveProperty('search');
2232
});
2333
});
2434

2535
describe('<Page>', () => {
26-
let props;
27-
2836
beforeEach(() => {
2937
trackPageView.mockReset();
30-
props = {
31-
pathname: String(Math.random()),
32-
search: String(Math.random()),
33-
};
34-
render(<Page {...props} />);
3538
});
3639

3740
it('renders without exploding', () => {
41+
renderWithPath({});
3842
expect(screen.getByRole('banner')).toBeInTheDocument();
3943
});
4044

41-
it('tracks an initial page-view', () => {
42-
const { pathname, search } = props;
43-
expect(trackPageView).toHaveBeenCalledWith(pathname, search);
45+
it('tracks an initial page-view using location from useLocation()', () => {
46+
renderWithPath({}, '/my-path?q=1');
47+
expect(trackPageView).toHaveBeenCalledWith('/my-path', '?q=1');
4448
});
4549

4650
it('tracks a pageView when the location changes', () => {
51+
const { rerender } = renderWithPath({}, '/first?a=1');
4752
trackPageView.mockReset();
48-
const newProps = { pathname: 'le-path', search: 'searching' };
49-
const { rerender } = render(<Page {...props} />);
50-
rerender(<Page {...newProps} />);
51-
expect(trackPageView).toHaveBeenCalledWith(newProps.pathname, newProps.search);
53+
// Use a different key to force the MemoryRouter to remount with the new initialEntries.
54+
rerender(
55+
<MemoryRouter key="router-2" initialEntries={['/second?b=2']}>
56+
<Page />
57+
</MemoryRouter>
58+
);
59+
expect(trackPageView).toHaveBeenCalledWith('/second', '?b=2');
5260
});
5361

5462
it('tracks a pageView when the search changes but pathname is same', () => {
63+
const { rerender } = renderWithPath({}, '/same-path?a=1');
5564
trackPageView.mockReset();
56-
const staticPathname = '/same-path';
57-
const { rerender } = render(<Page pathname={staticPathname} search="?a=1" />);
58-
rerender(<Page pathname={staticPathname} search="?a=2" />);
59-
expect(trackPageView).toHaveBeenCalledWith(staticPathname, '?a=2');
65+
rerender(
66+
<MemoryRouter key="router-2" initialEntries={['/same-path?a=2']}>
67+
<Page />
68+
</MemoryRouter>
69+
);
70+
expect(trackPageView).toHaveBeenCalledWith('/same-path', '?a=2');
6071
});
6172

6273
describe('Page embedded', () => {
6374
beforeEach(() => {
6475
trackPageView.mockReset();
65-
props = {
66-
pathname: String(Math.random()),
67-
search: 'hideGraph',
68-
};
69-
render(<Page embedded {...props} />);
76+
renderWithPath({ embedded: true });
7077
});
7178

7279
it('renders without exploding', () => {
73-
expect(screen.getByRole('banner')).toBeInTheDocument();
80+
// in embedded mode the Header/banner is hidden; check the content area instead.
81+
expect(screen.getByRole('main')).toBeInTheDocument();
7482
});
7583

7684
it('does not render Header', () => {
77-
expect(screen.queryByText('Header')).toBeNull();
85+
expect(screen.queryByRole('banner')).not.toBeInTheDocument();
7886
});
7987
});
8088
});

packages/jaeger-ui/src/components/App/Page.tsx

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import * as React from 'react';
55
import { Layout } from 'antd';
66
import cx from 'classnames';
77
import { connect } from 'react-redux';
8+
import { useLocation } from 'react-router-dom-v5-compat';
89

910
import TopNav from './TopNav';
1011
import { ReduxState } from '../../types';
@@ -18,14 +19,13 @@ import withRouteProps from '../../utils/withRouteProps';
1819
type TProps = {
1920
children: React.ReactNode;
2021
embedded: EmbeddedState;
21-
pathname: string;
22-
search: string;
2322
};
2423

2524
const { Header, Content } = Layout;
2625

2726
// export for tests
28-
export const PageImpl: React.FC<TProps> = ({ children, embedded, pathname, search }) => {
27+
export const PageImpl: React.FC<TProps> = ({ children, embedded }) => {
28+
const { pathname, search } = useLocation();
2929
React.useEffect(() => {
3030
trackPageView(pathname, search);
3131
}, [pathname, search]);
@@ -50,8 +50,7 @@ export const PageImpl: React.FC<TProps> = ({ children, embedded, pathname, searc
5050
// export for tests
5151
export function mapStateToProps(state: ReduxState) {
5252
const { embedded } = state;
53-
const { pathname, search } = state.router.location;
54-
return { embedded, pathname, search };
53+
return { embedded };
5554
}
5655

5756
export default connect(mapStateToProps)(withRouteProps(PageImpl));

packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/index.test.js

Lines changed: 42 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,25 @@ jest.mock('./calc-positioning', () => () => ({
88
svcMarginTop: 10,
99
}));
1010

11+
// Mutable object so individual tests can control the location without re-creating the mock.
12+
const mockLocation = { search: '' };
13+
14+
jest.mock('react-router-dom-v5-compat', () => ({
15+
...jest.requireActual('react-router-dom-v5-compat'),
16+
useLocation: () => mockLocation,
17+
}));
18+
19+
jest.mock('../../../../model/path-agnostic-decorations', () => ({
20+
__esModule: true,
21+
default: jest.fn(() => ({})),
22+
}));
23+
1124
import React from 'react';
1225
import { render, screen, fireEvent } from '@testing-library/react';
1326
import '@testing-library/jest-dom';
14-
15-
import {
27+
import { Provider } from 'react-redux';
28+
import extractDecorationFromState from '../../../../model/path-agnostic-decorations';
29+
import DdgNodeContentWrapper, {
1630
getNodeRenderer,
1731
measureNode,
1832
mapDispatchToProps,
@@ -676,4 +690,30 @@ describe('<DdgNodeContent>', () => {
676690
});
677691
});
678692
});
693+
694+
describe('DdgNodeContentWrapper (default export)', () => {
695+
const store = {
696+
getState: () => ({}),
697+
dispatch: jest.fn(),
698+
subscribe: jest.fn(() => jest.fn()),
699+
};
700+
701+
beforeEach(() => {
702+
extractDecorationFromState.mockClear();
703+
extractDecorationFromState.mockReturnValue({});
704+
});
705+
706+
it('injects search from useLocation() into the connected component', () => {
707+
mockLocation.search = '?decoration=some-id';
708+
render(
709+
<Provider store={store}>
710+
<DdgNodeContentWrapper {...props} />
711+
</Provider>
712+
);
713+
expect(extractDecorationFromState).toHaveBeenCalledWith(
714+
expect.any(Object),
715+
expect.objectContaining({ search: '?decoration=some-id' })
716+
);
717+
});
718+
});
679719
});

packages/jaeger-ui/src/components/DeepDependencies/Graph/DdgNodeContent/index.tsx

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import * as React from 'react';
55
import { Popover } from 'antd';
66
import cx from 'classnames';
7+
import { useLocation } from 'react-router-dom-v5-compat';
78
import { TLayoutVertex } from '@jaegertracing/plexus/lib/types';
89
import { IoLocate, IoEyeOff } from 'react-icons/io5';
910
import { connect } from 'react-redux';
@@ -396,6 +397,17 @@ export function mapDispatchToProps(dispatch: Dispatch<ReduxState>): TDispatchPro
396397
};
397398
}
398399

399-
const DdgNodeContent = connect(extractDecorationFromState, mapDispatchToProps)(UnconnectedDdgNodeContent);
400+
const ConnectedDdgNodeContent = connect(
401+
extractDecorationFromState,
402+
mapDispatchToProps
403+
)(UnconnectedDdgNodeContent);
404+
405+
// search is always injected from useLocation(); callers cannot supply it.
406+
type DdgNodeContentProps = Omit<React.ComponentProps<typeof ConnectedDdgNodeContent>, 'search'>;
407+
408+
function DdgNodeContent(props: DdgNodeContentProps) {
409+
const { search } = useLocation();
410+
return <ConnectedDdgNodeContent {...props} search={search} />;
411+
}
400412

401413
export default DdgNodeContent;

packages/jaeger-ui/src/components/DeepDependencies/Header/index.test.js

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -64,29 +64,22 @@ jest.mock('./LayoutSettings', () => {
6464

6565
jest.mock('../../common/UiFindInput', () => {
6666
const mockReact = jest.requireActual('react');
67-
return function MockUiFindInput(props) {
67+
return mockReact.forwardRef(function MockUiFindInput(props, ref) {
6868
const inputRef = mockReact.useRef(null);
6969

7070
mockReact.useEffect(() => {
71-
if (props.forwardedRef) {
72-
props.forwardedRef.current = {
73-
focus: () => {
74-
if (inputRef.current) {
75-
inputRef.current.focus();
76-
}
77-
},
78-
blur: () => {
79-
if (inputRef.current) {
80-
inputRef.current.blur();
81-
}
82-
},
83-
select: () => {
84-
if (inputRef.current) {
85-
inputRef.current.select();
86-
}
87-
},
71+
if (ref) {
72+
const current = {
73+
focus: () => inputRef.current && inputRef.current.focus(),
74+
blur: () => inputRef.current && inputRef.current.blur(),
75+
select: () => inputRef.current && inputRef.current.select(),
8876
input: inputRef.current,
8977
};
78+
if (typeof ref === 'function') {
79+
ref(current);
80+
} else {
81+
ref.current = current;
82+
}
9083
}
9184
});
9285

@@ -95,7 +88,7 @@ jest.mock('../../common/UiFindInput', () => {
9588
'data-testid': 'ui-find-input',
9689
...props.inputProps,
9790
});
98-
};
91+
});
9992
});
10093

10194
describe('<Header>', () => {

packages/jaeger-ui/src/components/DeepDependencies/Header/index.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -175,7 +175,7 @@ const Header: React.FC<TProps> = ({
175175
<IoSearch className="DdgHeader--uiFindSearchIcon" />
176176
<UiFindInput
177177
allowClear
178-
forwardedRef={uiFindInput}
178+
ref={uiFindInput}
179179
inputProps={{ className: 'DdgHeader--uiFindInput' }}
180180
trackFindFunction={trackFilter}
181181
/>

packages/jaeger-ui/src/components/DeepDependencies/SidePanel/DetailsPanel.test.js

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,30 @@
11
// Copyright (c) 2020 Uber Technologies, Inc.
22
// SPDX-License-Identifier: Apache-2.0
33

4+
// Mutable object so individual tests can control the location without re-creating the mock.
5+
const mockLocation = { search: '' };
6+
7+
jest.mock('react-router-dom-v5-compat', () => ({
8+
...jest.requireActual('react-router-dom-v5-compat'),
9+
useLocation: () => mockLocation,
10+
}));
11+
12+
jest.mock('../../../model/path-agnostic-decorations', () => ({
13+
__esModule: true,
14+
default: jest.fn(() => ({})),
15+
}));
16+
417
import React from 'react';
518
import { render, screen, fireEvent } from '@testing-library/react';
619
import '@testing-library/jest-dom';
720
import _set from 'lodash/set';
21+
import { Provider } from 'react-redux';
822

23+
import extractDecorationFromState from '../../../model/path-agnostic-decorations';
924
import stringSupplant from '../../../utils/stringSupplant';
1025
import JaegerAPI from '../../../api/jaeger';
1126
import { UnconnectedDetailsPanel as DetailsPanel } from './DetailsPanel';
27+
import DefaultDetailsPanel from './DetailsPanel';
1228

1329
jest.mock('../../common/VerticalResizer', () => {
1430
return ({ onChange, position }) => (
@@ -273,4 +289,34 @@ describe('<SidePanel>', () => {
273289
expect(container.querySelector('.Ddg--DetailsPanel')).toHaveStyle('width: 60vw');
274290
});
275291
});
292+
293+
describe('DetailsPanelWithLocation (default export)', () => {
294+
const store = {
295+
getState: () => ({}),
296+
dispatch: jest.fn(),
297+
subscribe: jest.fn(() => jest.fn()),
298+
};
299+
const wrapperProps = {
300+
decorationSchema: { name: 'Wrapper Test Schema' },
301+
service: 'wrapper-test-svc',
302+
};
303+
304+
beforeEach(() => {
305+
extractDecorationFromState.mockClear();
306+
extractDecorationFromState.mockReturnValue({});
307+
});
308+
309+
it('injects search from useLocation() into the connected component', () => {
310+
mockLocation.search = '?decoration=my-decoration-id';
311+
render(
312+
<Provider store={store}>
313+
<DefaultDetailsPanel {...wrapperProps} />
314+
</Provider>
315+
);
316+
expect(extractDecorationFromState).toHaveBeenCalledWith(
317+
expect.any(Object),
318+
expect.objectContaining({ search: '?decoration=my-decoration-id' })
319+
);
320+
});
321+
});
276322
});

packages/jaeger-ui/src/components/DeepDependencies/SidePanel/DetailsPanel.tsx

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
import * as React from 'react';
55
import { Tooltip } from 'antd';
6+
import { useLocation } from 'react-router-dom-v5-compat';
67
import _get from 'lodash/get';
78
import { connect } from 'react-redux';
89

@@ -180,4 +181,14 @@ function UnconnectedDetailsPanelImpl(props: TProps) {
180181

181182
export const UnconnectedDetailsPanel = React.memo(UnconnectedDetailsPanelImpl);
182183

183-
export default connect(extractDecorationFromState)(UnconnectedDetailsPanel);
184+
const ConnectedDetailsPanel = connect(extractDecorationFromState)(UnconnectedDetailsPanel);
185+
186+
// search is always injected from useLocation(); callers cannot supply it.
187+
type DetailsPanelWithLocationProps = Omit<React.ComponentProps<typeof ConnectedDetailsPanel>, 'search'>;
188+
189+
function DetailsPanelWithLocation(props: DetailsPanelWithLocationProps) {
190+
const { search } = useLocation();
191+
return <ConnectedDetailsPanel {...props} search={search} />;
192+
}
193+
194+
export default DetailsPanelWithLocation;

0 commit comments

Comments
 (0)