-
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 5 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,204 @@ | ||
| // 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 with render tracking | ||
| const mockSvgEdgeProps = []; | ||
| let mockRenderCount = 0; | ||
| jest.mock('./SvgEdge', () => { | ||
| const MockSvgEdge = props => { | ||
| mockSvgEdgeProps.push(props); | ||
| mockRenderCount++; | ||
| 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; | ||
| mockRenderCount = 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); | ||
| }); | ||
| }); | ||
|
|
||
| describe('memoization behavior', () => { | ||
| it('does not re-render when props remain the same', () => { | ||
| const { rerender } = render( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| const initialRenderCount = mockRenderCount; | ||
| expect(initialRenderCount).toBe(2); // 2 edges rendered | ||
|
|
||
| // Re-render with same props | ||
| rerender( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| // Render count should not increase due to memoization | ||
| expect(mockRenderCount).toBe(initialRenderCount); | ||
| }); | ||
|
|
||
| it('re-renders when layoutEdges changes', () => { | ||
| const { rerender } = render( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| const initialRenderCount = mockRenderCount; | ||
|
|
||
| // Create new edges array (different reference) | ||
| const newLayoutEdges = [createMockEdge('node-x', 'node-y')]; | ||
|
|
||
| rerender( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={newLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| // Render count should increase because layoutEdges changed | ||
| expect(mockRenderCount).toBeGreaterThan(initialRenderCount); | ||
| }); | ||
|
|
||
| it('re-renders when markerEndId changes', () => { | ||
| const { rerender } = render( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| markerEndId="arrow-end" | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| const initialRenderCount = mockRenderCount; | ||
|
|
||
| rerender( | ||
| <svg> | ||
| <SvgEdges | ||
| getClassName={mockGetClassName} | ||
| layoutEdges={mockLayoutEdges} | ||
| renderUtils={mockRenderUtils} | ||
| markerEndId="different-arrow" | ||
| /> | ||
| </svg> | ||
| ); | ||
|
|
||
| // Render count should increase because markerEndId changed | ||
| expect(mockRenderCount).toBeGreaterThan(initialRenderCount); | ||
| }); | ||
| }); | ||
| }); | ||
|
Comment on lines
+22
to
+326
|
||
| 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; | ||
Uh oh!
There was an error while loading. Please reload this page.