-
Notifications
You must be signed in to change notification settings - Fork 684
Convert SvgEdges to functional component #3402
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
Changes from 3 commits
512ed41
49b9cdf
6585beb
cd8d313
16917ef
a795906
f8a7851
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| // Copyright (c) 2026 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import React from 'react'; | ||
| import { render } from '@testing-library/react'; | ||
| import SvgEdges from './SvgEdges'; | ||
|
|
||
| // Mock SvgEdge child component | ||
| const mockSvgEdgeProps = []; | ||
| jest.mock('./SvgEdge', () => { | ||
| const MockSvgEdge = props => { | ||
| mockSvgEdgeProps.push(props); | ||
| return ( | ||
| <g data-testid="svg-edge" data-from={props.layoutEdge.edge.from} data-to={props.layoutEdge.edge.to} /> | ||
| ); | ||
| }; | ||
| return MockSvgEdge; | ||
| }); | ||
|
|
||
| describe('SvgEdges', () => { | ||
| beforeEach(() => { | ||
| mockSvgEdgeProps.length = 0; | ||
| }); | ||
|
|
||
| const mockGetClassName = name => `plexus--${name}`; | ||
|
|
||
| const mockRenderUtils = { | ||
| getGlobalId: id => `global-${id}`, | ||
| getZoomTransform: () => ({ k: 1, x: 0, y: 0 }), | ||
| }; | ||
|
|
||
| // Create mock edges for testing | ||
| const createMockEdge = (from, to) => ({ | ||
| edge: { from, to }, | ||
| pathPoints: [ | ||
| [0, 0], | ||
| [100, 100], | ||
| ], | ||
| }); | ||
|
|
||
| const mockLayoutEdges = [createMockEdge('node-a', 'node-b'), createMockEdge('node-b', 'node-c')]; | ||
|
|
||
| // Wrap in svg container since SvgEdge renders SVG elements | ||
| const renderInSvg = edges => { | ||
| return render( | ||
| <svg> | ||
| <SvgEdges getClassName={mockGetClassName} layoutEdges={edges} renderUtils={mockRenderUtils} /> | ||
| </svg> | ||
| ); | ||
| }; | ||
|
|
||
| it('renders SvgEdge for each layout edge', () => { | ||
| const { getAllByTestId } = renderInSvg(mockLayoutEdges); | ||
| const edges = getAllByTestId('svg-edge'); | ||
| expect(edges).toHaveLength(2); | ||
| }); | ||
|
|
||
| it('renders nothing when layoutEdges is empty', () => { | ||
| const { queryAllByTestId } = renderInSvg([]); | ||
| const edges = queryAllByTestId('svg-edge'); | ||
| expect(edges).toHaveLength(0); | ||
| }); | ||
|
|
||
| it('passes from and to correctly to SvgEdge', () => { | ||
| const { getAllByTestId } = renderInSvg(mockLayoutEdges); | ||
| const edges = getAllByTestId('svg-edge'); | ||
|
|
||
| // Check first edge | ||
| expect(edges[0]).toHaveAttribute('data-from', 'node-a'); | ||
| expect(edges[0]).toHaveAttribute('data-to', 'node-b'); | ||
|
|
||
| // Check second edge | ||
| expect(edges[1]).toHaveAttribute('data-from', 'node-b'); | ||
| expect(edges[1]).toHaveAttribute('data-to', 'node-c'); | ||
| }); | ||
|
|
||
| it('handles optional marker props', () => { | ||
| const { getAllByTestId } = render( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| markerEndId="arrow-end" | ||
| markerStartId="arrow-start" | ||
| /> | ||
| </svg> | ||
| ); | ||
| expect(getAllByTestId('svg-edge')).toHaveLength(2); | ||
| }); | ||
|
|
||
| it('passes setOnEdge to child components', () => { | ||
| const mockSetOnEdge = jest.fn(); | ||
| const { getAllByTestId } = render( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| setOnEdge={mockSetOnEdge} | ||
| /> | ||
| </svg> | ||
| ); | ||
| expect(getAllByTestId('svg-edge')).toHaveLength(2); | ||
|
|
||
| // Verify setOnEdge is passed to each child component | ||
| mockSvgEdgeProps.forEach(props => { | ||
| expect(props.setOnEdge).toBe(mockSetOnEdge); | ||
| }); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| // Copyright (c) 2019 Uber Technologies, Inc. | ||
| // Copyright (c) 2026 The Jaeger Authors. | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| import * as React from 'react'; | ||
|
|
@@ -17,32 +17,39 @@ type TProps<T = {}> = { | |
| setOnEdge?: TSetProps<(edge: TLayoutEdge<T>, utils: TRendererUtils) => TAnyProps | null>; | ||
| }; | ||
|
|
||
| export default class SvgEdges<T = {}> extends React.Component<TProps<T>> { | ||
| shouldComponentUpdate(np: TProps<T>) { | ||
| const p = this.props; | ||
| return ( | ||
| p.getClassName !== np.getClassName || | ||
| p.layoutEdges !== np.layoutEdges || | ||
| p.markerEndId !== np.markerEndId || | ||
| p.markerStartId !== np.markerStartId || | ||
| p.renderUtils !== np.renderUtils || | ||
| !isSamePropSetter(p.setOnEdge, np.setOnEdge) | ||
| ); | ||
| } | ||
|
|
||
| render() { | ||
| const { getClassName, layoutEdges, markerEndId, markerStartId, renderUtils, setOnEdge } = this.props; | ||
| return layoutEdges.map(edge => ( | ||
| <SvgEdge | ||
| key={`${edge.edge.from}\v${edge.edge.to}`} | ||
| getClassName={getClassName} | ||
| layoutEdge={edge} | ||
| markerEndId={markerEndId} | ||
| markerStartId={markerStartId} | ||
| renderUtils={renderUtils} | ||
| setOnEdge={setOnEdge} | ||
| label={edge.edge.label} | ||
| /> | ||
| )); | ||
| } | ||
| // Comparison function that mirrors the original shouldComponentUpdate logic | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why can't we simply rely on React doing the comparison? That is literally the reason why React was created.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the delayed reply, I was at KubeCon EU these past two weeks. Thank you for the review. You're right, the custom comparator shouldn't be here. I raised the same point on #3403 and offered to simplify, but should have just gone ahead and done it. The isSamePropSetter call on setOnEdge is handling something the caller should stabilize with useMemo, not something the child should work around. I'll switch to plain React.memo() here and on #3403. Both need a rebase against the toolchain changes anyway, so I'll handle them together. |
||
| // Returns true if props are equal (no re-render needed) | ||
| function arePropsEqual<T>(prev: TProps<T>, next: TProps<T>): boolean { | ||
| return ( | ||
| prev.getClassName === next.getClassName && | ||
| prev.layoutEdges === next.layoutEdges && | ||
| prev.markerEndId === next.markerEndId && | ||
| prev.markerStartId === next.markerStartId && | ||
| prev.renderUtils === next.renderUtils && | ||
| isSamePropSetter(prev.setOnEdge, next.setOnEdge) | ||
| ); | ||
| } | ||
|
|
||
| const SvgEdges = <T = {},>({ | ||
| getClassName, | ||
| layoutEdges, | ||
| markerEndId, | ||
| markerStartId, | ||
| renderUtils, | ||
| setOnEdge, | ||
| }: TProps<T>) => { | ||
| return layoutEdges.map(edge => ( | ||
| <SvgEdge | ||
| key={`${edge.edge.from}\v${edge.edge.to}`} | ||
| getClassName={getClassName} | ||
| layoutEdge={edge} | ||
| markerEndId={markerEndId} | ||
| markerStartId={markerStartId} | ||
| renderUtils={renderUtils} | ||
| setOnEdge={setOnEdge} | ||
| label={edge.edge.label} | ||
| /> | ||
| )); | ||
| }; | ||
|
|
||
| export default React.memo(SvgEdges, arePropsEqual) as unknown as typeof SvgEdges; | ||
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.
The tests do not verify the memoization behavior of the React.memo wrapper. Since the conversion from a class component with shouldComponentUpdate to a functional component with React.memo is a key aspect of this PR, consider adding tests that verify the component does not re-render when props haven't changed according to the arePropsEqual logic. This could involve using the rerender function from testing-library and tracking render counts.