From 0699a04dcd462a856f0b1c219333fa2196982dc3 Mon Sep 17 00:00:00 2001 From: Connor Lindsey Date: Mon, 2 Aug 2021 08:03:08 -0600 Subject: [PATCH] Refactor UNSAFE_component... from SpanGraph and VirtualizedTraceView (#37418) --- .../TracePageHeader/SpanGraph/index.test.js | 12 ++ .../src/TracePageHeader/SpanGraph/index.tsx | 45 ++----- .../VirtualizedTraceView.test.js | 3 +- .../VirtualizedTraceView.tsx | 127 +++++++++--------- 4 files changed, 93 insertions(+), 94 deletions(-) diff --git a/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.test.js b/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.test.js index 52d9bc6c3d7..4ef74638ee6 100644 --- a/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.test.js +++ b/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.test.js @@ -73,4 +73,16 @@ describe('', () => { })); expect(canvasGraph.prop('items')).toEqual(items); }); + + it('does not regenerate CanvasSpanGraph without new trace', () => { + const canvasGraph = wrapper.find(CanvasSpanGraph).first(); + const items = canvasGraph.prop('items'); + + wrapper.instance().forceUpdate(); + + const newCanvasGraph = wrapper.find(CanvasSpanGraph).first(); + const newItems = newCanvasGraph.prop('items'); + + expect(newItems).toBe(items); + }); }); diff --git a/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.tsx b/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.tsx index 0677463300f..4ddae074538 100644 --- a/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.tsx +++ b/packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.tsx @@ -14,6 +14,7 @@ import * as React from 'react'; import cx from 'classnames'; +import memoizeOne from 'memoize-one'; import CanvasSpanGraph from './CanvasSpanGraph'; import TickLabels from './TickLabels'; @@ -33,19 +34,13 @@ type SpanGraphProps = { updateNextViewRangeTime: (nextUpdate: ViewRangeTimeUpdate) => void; }; -/** - * Store `items` in state so they are not regenerated every render. Otherwise, - * the canvas graph will re-render itself every time. - */ -type SpanGraphState = { - items: Array<{ - valueOffset: number; - valueWidth: number; - serviceName: string; - }>; +type SpanItem = { + valueOffset: number; + valueWidth: number; + serviceName: string; }; -function getItem(span: TraceSpan) { +function getItem(span: TraceSpan): SpanItem { return { valueOffset: span.relativeStartTime, valueWidth: span.duration, @@ -53,36 +48,24 @@ function getItem(span: TraceSpan) { }; } -export default class SpanGraph extends React.PureComponent { - state: SpanGraphState; +function getItems(trace: Trace): SpanItem[] { + return trace.spans.map(getItem); +} +const memoizedGetitems = memoizeOne(getItems); + +export default class SpanGraph extends React.PureComponent { static defaultProps = { height: DEFAULT_HEIGHT, }; - constructor(props: SpanGraphProps) { - super(props); - const { trace } = props; - this.state = { - items: trace ? trace.spans.map(getItem) : [], - }; - } - - UNSAFE_componentWillReceiveProps(nextProps: SpanGraphProps) { - const { trace } = nextProps; - if (this.props.trace !== trace) { - this.setState({ - items: trace ? trace.spans.map(getItem) : [], - }); - } - } - render() { const { height, trace, viewRange, updateNextViewRangeTime, updateViewRangeTime } = this.props; if (!trace) { return
; } - const { items } = this.state; + + const items = memoizedGetitems(trace); return (
diff --git a/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.test.js b/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.test.js index ed64e23e186..73dea7818bc 100644 --- a/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.test.js +++ b/packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.test.js @@ -300,7 +300,8 @@ describe('', () => { expect( rowWrapper.containsMatchingElement( , + detailStates: Map +): RowState[] { + return trace ? generateRowStates(trace.spans, childrenHiddenIDs, detailStates) : []; +} + +const memoizedGenerateRowStates = memoizeOne(generateRowStatesFromTrace); +const memoizedViewBoundsFunc = memoizeOne(createViewedBoundsFunc, isEqual); +const memoizedGetClipping = memoizeOne(getClipping, isEqual); + // export from tests export class UnthemedVirtualizedTraceView extends React.Component { - clipping: { left: boolean; right: boolean }; listView: ListView | TNil; - rowStates: RowState[]; - getViewedBounds: ViewedBoundsFunctionType; constructor(props: VirtualizedTraceViewProps) { super(props); - // keep "prop derivations" on the instance instead of calculating in - // `.render()` to avoid recalculating in every invocation of `.renderRow()` - const { currentViewRangeTime, childrenHiddenIDs, detailStates, setTrace, trace, uiFind } = props; - this.clipping = getClipping(currentViewRangeTime); - const [zoomStart, zoomEnd] = currentViewRangeTime; - this.getViewedBounds = createViewedBoundsFunc({ - min: trace.startTime, - max: trace.endTime, - viewStart: zoomStart, - viewEnd: zoomEnd, - }); - this.rowStates = generateRowStates(trace.spans, childrenHiddenIDs, detailStates); - + const { setTrace, trace, uiFind } = props; setTrace(trace, uiFind); } @@ -191,50 +190,54 @@ export class UnthemedVirtualizedTraceView extends React.Component) { + const { registerAccessors, trace } = prevProps; const { shouldScrollToFirstUiFindMatch, clearShouldScrollToFirstUiFindMatch, scrollToFirstVisibleSpan, + registerAccessors: nextRegisterAccessors, + setTrace, + trace: nextTrace, + uiFind, } = this.props; + + if (trace !== nextTrace) { + setTrace(nextTrace, uiFind); + } + + if (this.listView && registerAccessors !== nextRegisterAccessors) { + nextRegisterAccessors(this.getAccessors()); + } + if (shouldScrollToFirstUiFindMatch) { scrollToFirstVisibleSpan(); clearShouldScrollToFirstUiFindMatch(); } } + getRowStates(): RowState[] { + const { childrenHiddenIDs, detailStates, trace } = this.props; + return memoizedGenerateRowStates(trace, childrenHiddenIDs, detailStates); + } + + getClipping(): { left: boolean; right: boolean } { + const { currentViewRangeTime } = this.props; + return memoizedGetClipping(currentViewRangeTime); + } + + getViewedBounds(): ViewedBoundsFunctionType { + const { currentViewRangeTime, trace } = this.props; + const [zoomStart, zoomEnd] = currentViewRangeTime; + + return memoizedViewBoundsFunc({ + min: trace.startTime, + max: trace.endTime, + viewStart: zoomStart, + viewEnd: zoomEnd, + }); + } + getAccessors() { const lv = this.listView; if (!lv) { @@ -259,12 +262,12 @@ export class UnthemedVirtualizedTraceView extends React.Component this.props.childrenHiddenIDs; - mapRowIndexToSpanIndex = (index: number) => this.rowStates[index].spanIndex; + mapRowIndexToSpanIndex = (index: number) => this.getRowStates()[index].spanIndex; mapSpanIndexToRowIndex = (index: number) => { - const max = this.rowStates.length; + const max = this.getRowStates().length; for (let i = 0; i < max; i++) { - const { spanIndex } = this.rowStates[i]; + const { spanIndex } = this.getRowStates()[i]; if (spanIndex === index) { return i; } @@ -283,7 +286,7 @@ export class UnthemedVirtualizedTraceView extends React.Component { - const { isDetail, span } = this.rowStates[index]; + const { isDetail, span } = this.getRowStates()[index]; return `${span.spanID}--${isDetail ? 'detail' : 'bar'}`; }; @@ -291,9 +294,9 @@ export class UnthemedVirtualizedTraceView extends React.Component { - const { span, isDetail } = this.rowStates[index]; + const { span, isDetail } = this.getRowStates()[index]; if (!isDetail) { return DEFAULT_HEIGHTS.bar; } @@ -313,7 +316,7 @@ export class UnthemedVirtualizedTraceView extends React.Component { - const { isDetail, span, spanIndex } = this.rowStates[index]; + const { isDetail, span, spanIndex } = this.getRowStates()[index]; return isDetail ? this.renderSpanDetailRow(span, key, style, attrs) : this.renderSpanBarRow(span, spanIndex, key, style, attrs); @@ -352,7 +355,7 @@ export class UnthemedVirtualizedTraceView extends React.Component