Refactor UNSAFE_component... from SpanGraph and VirtualizedTraceView (#37418)

packages/jaeger-ui-components/src/TracePageHeader/SpanGraph/index.test.js

@@ -73,4 +73,16 @@ describe('<SpanGraph>', () => {
     }));
     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);
+  });
 });

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;
 };
 
-/**
+type SpanItem = {
- * 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;
-  }>;
 };
 
-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<SpanGraphProps, SpanGraphState> {
+function getItems(trace: Trace): SpanItem[] {
-  state: SpanGraphState;
+  return trace.spans.map(getItem);
+}
 
+const memoizedGetitems = memoizeOne(getItems);
+
+export default class SpanGraph extends React.PureComponent<SpanGraphProps> {
   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 <div />;
     }
-    const { items } = this.state;
+
+    const items = memoizedGetitems(trace);
     return (
       <div className={cx(ubPb2, ubPx2)}>
         <TickLabels numTicks={TIMELINE_TICK_INTERVAL} duration={trace.duration} />

packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.test.js

@@ -300,7 +300,8 @@ describe('<VirtualizedTraceViewImpl>', () => {
       expect(
         rowWrapper.containsMatchingElement(
           <SpanBarRow
-            className={instance.clippingCssClasses}
+            clippingLeft={instance.getClipping().left}
+            clippingRight={instance.getClipping().right}
             columnDivision={props.spanNameColumnWidth}
             isChildrenExpanded
             isDetailExpanded={false}

packages/jaeger-ui-components/src/TraceTimelineViewer/VirtualizedTraceView.tsx

@@ -15,6 +15,8 @@
 import * as React from 'react';
 import { css } from '@emotion/css';
 
+import { isEqual } from 'lodash';
+import memoizeOne from 'memoize-one';
 import ListView from './ListView';
 import SpanBarRow from './SpanBarRow';
 import DetailState from './SpanDetail/DetailState';
@@ -148,28 +150,25 @@ function getClipping(currentViewRange: [number, number]) {
   };
 }
 
+function generateRowStatesFromTrace(
+  trace: Trace | TNil,
+  childrenHiddenIDs: Set<string>,
+  detailStates: Map<string, DetailState | TNil>
+): 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<VirtualizedTraceViewProps> {
-  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
+    const { setTrace, trace, uiFind } = props;
-    // `.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);
-
     setTrace(trace, uiFind);
   }
 
@@ -191,50 +190,54 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
     return false;
   }
 
-  UNSAFE_componentWillUpdate(nextProps: VirtualizedTraceViewProps) {
+  componentDidUpdate(prevProps: Readonly<VirtualizedTraceViewProps>) {
-    const { childrenHiddenIDs, detailStates, registerAccessors, trace, currentViewRangeTime } = this.props;
+    const { registerAccessors, trace } = prevProps;
-    const {
-      currentViewRangeTime: nextViewRangeTime,
-      childrenHiddenIDs: nextHiddenIDs,
-      detailStates: nextDetailStates,
-      registerAccessors: nextRegisterAccessors,
-      setTrace,
-      trace: nextTrace,
-      uiFind,
-    } = nextProps;
-    if (trace !== nextTrace) {
-      setTrace(nextTrace, uiFind);
-    }
-    if (trace !== nextTrace || childrenHiddenIDs !== nextHiddenIDs || detailStates !== nextDetailStates) {
-      this.rowStates = nextTrace ? generateRowStates(nextTrace.spans, nextHiddenIDs, nextDetailStates) : [];
-    }
-    if (currentViewRangeTime !== nextViewRangeTime || (trace !== nextTrace && nextTrace)) {
-      this.clipping = getClipping(nextViewRangeTime);
-      const [zoomStart, zoomEnd] = nextViewRangeTime;
-      this.getViewedBounds = createViewedBoundsFunc({
-        min: nextTrace.startTime,
-        max: nextTrace.endTime,
-        viewStart: zoomStart,
-        viewEnd: zoomEnd,
-      });
-    }
-    if (this.listView && registerAccessors !== nextRegisterAccessors) {
-      nextRegisterAccessors(this.getAccessors());
-    }
-  }
-
-  componentDidUpdate() {
     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<VirtualizedTra
 
   getCollapsedChildren = () => 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<VirtualizedTra
   // use long form syntax to avert flow error
   // https://github.com/facebook/flow/issues/3076#issuecomment-290944051
   getKeyFromIndex = (index: number) => {
-    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<VirtualizedTra
     const parts = key.split('--');
     const _spanID = parts[0];
     const _isDetail = parts[1] === 'detail';
-    const max = this.rowStates.length;
+    const max = this.getRowStates().length;
     for (let i = 0; i < max; i++) {
-      const { span, isDetail } = this.rowStates[i];
+      const { span, isDetail } = this.getRowStates()[i];
       if (span.spanID === _spanID && isDetail === _isDetail) {
         return i;
       }
@@ -302,7 +305,7 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
   };
 
   getRowHeight = (index: number) => {
-    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<VirtualizedTra
   };
 
   renderRow = (key: string, style: React.CSSProperties, index: number, attrs: {}) => {
-    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<VirtualizedTra
     if (isCollapsed) {
       const rpcSpan = findServerChildSpan(trace.spans.slice(spanIndex));
       if (rpcSpan) {
-        const rpcViewBounds = this.getViewedBounds(rpcSpan.startTime, rpcSpan.startTime + rpcSpan.duration);
+        const rpcViewBounds = this.getViewedBounds()(rpcSpan.startTime, rpcSpan.startTime + rpcSpan.duration);
         rpc = {
           color: getColorByKey(rpcSpan.process.serviceName, theme),
           operationName: rpcSpan.operationName,
@@ -378,8 +381,8 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
     return (
       <div className={styles.row} key={key} style={style} {...attrs}>
         <SpanBarRow
-          clippingLeft={this.clipping.left}
+          clippingLeft={this.getClipping().left}
-          clippingRight={this.clipping.right}
+          clippingRight={this.getClipping().right}
           color={color}
           columnDivision={spanNameColumnWidth}
           isChildrenExpanded={!isCollapsed}
@@ -391,7 +394,7 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
           rpc={rpc}
           noInstrumentedServer={noInstrumentedServer}
           showErrorIcon={showErrorIcon}
-          getViewedBounds={this.getViewedBounds}
+          getViewedBounds={this.getViewedBounds()}
           traceStartTime={trace.startTime}
           span={span}
           focusSpan={focusSpan}
@@ -467,7 +470,7 @@ export class UnthemedVirtualizedTraceView extends React.Component<VirtualizedTra
       <div>
         <ListView
           ref={this.setListView}
-          dataLength={this.rowStates.length}
+          dataLength={this.getRowStates().length}
           itemHeightGetter={this.getRowHeight}
           itemRenderer={this.renderRow}
           viewBuffer={50}