From 1e74037eae6627f4891899ad2cbc8bfcad1e8f56 Mon Sep 17 00:00:00 2001
From: Boyko <boyskila@users.noreply.github.com>
Date: Mon, 18 May 2020 19:03:29 +0300
Subject: [PATCH] React: refactor away from unsafe lifecycle methods (#21291)

* refactor aliasBy and PopoverCtrl components

* refactor Aggregations component

* refactor MetricSelect component

* refactor UserProvider

* popoverCtr: remove redundant logic

* simplified the MetricsSelect a bit.

* skipping testing of internal component logic.

* changed to componentWillMount.

* changed elapsed time to a functional component.

* rewrote the tests.

* fixed missing test title.

* fixed a tiny issue with elapsed time.

* rename of field.

Co-authored-by: Marcus Andersson <marcus.andersson@grafana.com>
---
 .../components/Tooltip/PopoverController.tsx  |  52 +------
 .../components/Select/MetricSelect.test.tsx   |  48 ++++++
 .../core/components/Select/MetricSelect.tsx   | 110 ++++++--------
 public/app/features/explore/ElapsedTime.tsx   |  78 ++--------
 public/app/features/explore/LiveLogs.tsx      |   2 +-
 public/app/features/explore/QueryStatus.tsx   |   2 +-
 public/app/features/explore/Time.tsx          |  35 +++++
 .../components/Aggregations.test.tsx          |  55 ++++---
 .../stackdriver/components/Aggregations.tsx   | 138 ++++++++----------
 9 files changed, 242 insertions(+), 278 deletions(-)
 create mode 100644 public/app/core/components/Select/MetricSelect.test.tsx
 create mode 100644 public/app/features/explore/Time.tsx

diff --git a/packages/grafana-ui/src/components/Tooltip/PopoverController.tsx b/packages/grafana-ui/src/components/Tooltip/PopoverController.tsx
index 4adcde79bde..8d3ee22a412 100644
--- a/packages/grafana-ui/src/components/Tooltip/PopoverController.tsx
+++ b/packages/grafana-ui/src/components/Tooltip/PopoverController.tsx
@@ -48,63 +48,27 @@ interface Props {
 }
 
 interface State {
-  placement: PopperJS.Placement;
   show: boolean;
 }
 
 class PopoverController extends React.Component<Props, State> {
   private hideTimeout: any;
-
-  constructor(props: Props) {
-    super(props);
-
-    this.state = {
-      placement: this.props.placement || 'auto',
-      show: false,
-    };
-  }
-
-  UNSAFE_componentWillReceiveProps(nextProps: Props) {
-    if (nextProps.placement && nextProps.placement !== this.state.placement) {
-      this.setState((prevState: State) => {
-        return {
-          ...prevState,
-          placement: nextProps.placement || 'auto',
-        };
-      });
-    }
-  }
+  state = { show: false };
 
   showPopper = () => {
-    if (this.hideTimeout) {
-      clearTimeout(this.hideTimeout);
-    }
-
-    this.setState(prevState => ({
-      ...prevState,
-      show: true,
-    }));
+    clearTimeout(this.hideTimeout);
+    this.setState({ show: true });
   };
 
   hidePopper = () => {
-    if (this.props.hideAfter !== 0) {
-      this.hideTimeout = setTimeout(() => {
-        this.setState(prevState => ({
-          ...prevState,
-          show: false,
-        }));
-      }, this.props.hideAfter);
-      return;
-    }
-    this.setState(prevState => ({
-      ...prevState,
-      show: false,
-    }));
+    this.hideTimeout = setTimeout(() => {
+      this.setState({ show: false });
+    }, this.props.hideAfter);
   };
 
   render() {
-    const { children, content } = this.props;
-    const { show, placement } = this.state;
+    const { children, content, placement = 'auto' } = this.props;
+    const { show } = this.state;
 
     return children(this.showPopper, this.hidePopper, {
       show,
diff --git a/public/app/core/components/Select/MetricSelect.test.tsx b/public/app/core/components/Select/MetricSelect.test.tsx
new file mode 100644
index 00000000000..9a5165a2f53
--- /dev/null
+++ b/public/app/core/components/Select/MetricSelect.test.tsx
@@ -0,0 +1,48 @@
+import React from 'react';
+import { shallow } from 'enzyme';
+import { MetricSelect } from './MetricSelect';
+import { LegacyForms } from '@grafana/ui';
+const { Select } = LegacyForms;
+
+describe('MetricSelect', () => {
+  describe('When receive props', () => {
+    it('should pass correct set of props to Select component', () => {
+      const props: any = {
+        placeholder: 'Select Reducer',
+        className: 'width-15',
+        options: [],
+        variables: [],
+      };
+      const wrapper = shallow(<MetricSelect {...props} />);
+      expect(wrapper.find(Select).props()).toMatchObject({
+        className: 'width-15',
+        isMulti: false,
+        isClearable: false,
+        backspaceRemovesValue: false,
+        isSearchable: true,
+        maxMenuHeight: 500,
+        placeholder: 'Select Reducer',
+      });
+    });
+    it('should pass callbacks correctly to the Select component', () => {
+      const spyOnChange = jest.fn();
+      const props: any = {
+        onChange: spyOnChange,
+        options: [],
+        variables: [],
+      };
+      const wrapper = shallow(<MetricSelect {...props} />);
+      wrapper
+        .find(Select)
+        .props()
+        .onChange({ value: 'foo' });
+      expect(
+        wrapper
+          .find(Select)
+          .props()
+          .noOptionsMessage()
+      ).toEqual('No options found');
+      expect(spyOnChange).toHaveBeenCalledWith('foo');
+    });
+  });
+});
diff --git a/public/app/core/components/Select/MetricSelect.tsx b/public/app/core/components/Select/MetricSelect.tsx
index a726e470ac0..37bee34db3b 100644
--- a/public/app/core/components/Select/MetricSelect.tsx
+++ b/public/app/core/components/Select/MetricSelect.tsx
@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { useMemo, useCallback, FC } from 'react';
 import _ from 'lodash';
 
 import { LegacyForms } from '@grafana/ui';
@@ -16,75 +16,51 @@ export interface Props {
   variables?: Variable[];
 }
 
-interface State {
-  options: Array<SelectableValue<string>>;
-}
+export const MetricSelect: FC<Props> = props => {
+  const { value, placeholder, className, isSearchable, onChange } = props;
+  const options = useSelectOptions(props);
+  const selected = useSelectedOption(options, value);
+  const onChangeValue = useCallback((selectable: SelectableValue<string>) => onChange(selectable.value), [onChange]);
 
-export class MetricSelect extends React.Component<Props, State> {
-  static defaultProps: Partial<Props> = {
-    variables: [],
-    options: [],
-    isSearchable: true,
-  };
+  return (
+    <Select
+      className={className}
+      isMulti={false}
+      isClearable={false}
+      backspaceRemovesValue={false}
+      onChange={onChangeValue}
+      options={options}
+      isSearchable={isSearchable}
+      maxMenuHeight={500}
+      placeholder={placeholder}
+      noOptionsMessage={() => 'No options found'}
+      value={selected}
+    />
+  );
+};
 
-  constructor(props: Props) {
-    super(props);
-    this.state = { options: [] };
-  }
-
-  componentDidMount() {
-    this.setState({ options: this.buildOptions(this.props) });
-  }
-
-  UNSAFE_componentWillReceiveProps(nextProps: Props) {
-    if (nextProps.options.length > 0 || nextProps.variables?.length) {
-      this.setState({ options: this.buildOptions(nextProps) });
+const useSelectOptions = ({ variables = [], options }: Props): Array<SelectableValue<string>> => {
+  return useMemo(() => {
+    if (!Array.isArray(variables) || variables.length === 0) {
+      return options;
     }
-  }
 
-  shouldComponentUpdate(nextProps: Props) {
-    const nextOptions = this.buildOptions(nextProps);
-    return nextProps.value !== this.props.value || !_.isEqual(nextOptions, this.state.options);
-  }
+    return [
+      {
+        label: 'Template Variables',
+        options: variables.map(({ name }) => ({
+          label: `$${name}`,
+          value: `$${name}`,
+        })),
+      },
+      ...options,
+    ];
+  }, [variables, options]);
+};
 
-  buildOptions({ variables = [], options }: Props) {
-    return variables.length > 0 ? [this.getVariablesGroup(), ...options] : options;
-  }
-
-  getVariablesGroup() {
-    return {
-      label: 'Template Variables',
-      options: this.props.variables?.map(v => ({
-        label: `$${v.name}`,
-        value: `$${v.name}`,
-      })),
-    };
-  }
-
-  getSelectedOption() {
-    const { options } = this.state;
+const useSelectedOption = (options: Array<SelectableValue<string>>, value: string): SelectableValue<string> => {
+  return useMemo(() => {
     const allOptions = options.every(o => o.options) ? _.flatten(options.map(o => o.options)) : options;
-    return allOptions.find(option => option.value === this.props.value);
-  }
-
-  render() {
-    const { placeholder, className, isSearchable, onChange } = this.props;
-    const { options } = this.state;
-    const selectedOption = this.getSelectedOption();
-    return (
-      <Select
-        className={className}
-        isMulti={false}
-        isClearable={false}
-        backspaceRemovesValue={false}
-        onChange={item => onChange(item.value ?? '')}
-        options={options}
-        isSearchable={isSearchable}
-        maxMenuHeight={500}
-        placeholder={placeholder}
-        noOptionsMessage={() => 'No options found'}
-        value={selectedOption}
-      />
-    );
-  }
-}
+    return allOptions.find(option => option.value === value);
+  }, [options, value]);
+};
diff --git a/public/app/features/explore/ElapsedTime.tsx b/public/app/features/explore/ElapsedTime.tsx
index ab50cc12472..c358a0048bf 100644
--- a/public/app/features/explore/ElapsedTime.tsx
+++ b/public/app/features/explore/ElapsedTime.tsx
@@ -1,76 +1,22 @@
-import React, { PureComponent } from 'react';
-import { toDuration } from '@grafana/data';
+import React, { FC, useState, useEffect } from 'react';
+import { useInterval } from 'react-use';
+import { Time, TimeProps } from './Time';
 
 const INTERVAL = 150;
 
-export interface Props {
-  time?: number;
+export interface ElapsedTimeProps extends Omit<TimeProps, 'timeInMs'> {
   // Use this to reset the timer. Any value is allowed just need to be !== from the previous.
   // Keep in mind things like [] !== [] or {} !== {}.
   resetKey?: any;
-  className?: string;
-  humanize?: boolean;
 }
 
-export interface State {
-  elapsed: number;
-}
+export const ElapsedTime: FC<ElapsedTimeProps> = ({ resetKey, humanize, className }) => {
+  const [elapsed, setElapsed] = useState(0); // the current value of elapsed
 
-/**
- * Shows an incremental time ticker of elapsed time from some event.
- */
-export default class ElapsedTime extends PureComponent<Props, State> {
-  offset: number;
-  timer: number;
+  // hook that will schedule a interval and then update the elapsed value on every tick.
+  useInterval(() => setElapsed(elapsed + INTERVAL), INTERVAL);
+  // this effect will only be run when resetKey changes. This will reset the elapsed to 0.
+  useEffect(() => setElapsed(0), [resetKey]);
 
-  state = {
-    elapsed: 0,
-  };
-
-  start() {
-    this.offset = Date.now();
-    this.timer = window.setInterval(this.tick, INTERVAL);
-  }
-
-  tick = () => {
-    const jetzt = Date.now();
-    const elapsed = jetzt - this.offset;
-    this.setState({ elapsed });
-  };
-
-  UNSAFE_componentWillReceiveProps(nextProps: Props) {
-    if (nextProps.time) {
-      clearInterval(this.timer);
-    } else if (this.props.time) {
-      this.start();
-    }
-
-    if (nextProps.resetKey !== this.props.resetKey) {
-      clearInterval(this.timer);
-      this.start();
-    }
-  }
-
-  componentDidMount() {
-    this.start();
-  }
-
-  componentWillUnmount() {
-    clearInterval(this.timer);
-  }
-
-  render() {
-    const { elapsed } = this.state;
-    const { className, time, humanize } = this.props;
-    const value = (time || elapsed) / 1000;
-    let displayValue = `${value.toFixed(1)}s`;
-    if (humanize) {
-      const duration = toDuration(elapsed);
-      const hours = duration.hours();
-      const minutes = duration.minutes();
-      const seconds = duration.seconds();
-      displayValue = hours ? `${hours}h ${minutes}m ${seconds}s` : minutes ? ` ${minutes}m ${seconds}s` : `${seconds}s`;
-    }
-    return <span className={`elapsed-time ${className}`}>{displayValue}</span>;
-  }
-}
+  return <Time timeInMs={elapsed} className={className} humanize={humanize} />;
+};
diff --git a/public/app/features/explore/LiveLogs.tsx b/public/app/features/explore/LiveLogs.tsx
index 9afd24c83a8..73405d6cc6e 100644
--- a/public/app/features/explore/LiveLogs.tsx
+++ b/public/app/features/explore/LiveLogs.tsx
@@ -5,7 +5,7 @@ import tinycolor from 'tinycolor2';
 import { Themeable, withTheme, getLogRowStyles, Icon } from '@grafana/ui';
 import { GrafanaTheme, LogRowModel, TimeZone, dateTimeFormat } from '@grafana/data';
 
-import ElapsedTime from './ElapsedTime';
+import { ElapsedTime } from './ElapsedTime';
 
 const getStyles = (theme: GrafanaTheme) => ({
   logsRowsLive: css`
diff --git a/public/app/features/explore/QueryStatus.tsx b/public/app/features/explore/QueryStatus.tsx
index 1d88db8c1c5..4ff6e845d1a 100644
--- a/public/app/features/explore/QueryStatus.tsx
+++ b/public/app/features/explore/QueryStatus.tsx
@@ -1,6 +1,6 @@
 import React, { PureComponent } from 'react';
 
-import ElapsedTime from './ElapsedTime';
+import { ElapsedTime } from './ElapsedTime';
 import { PanelData, LoadingState } from '@grafana/data';
 
 function formatLatency(value: number) {
diff --git a/public/app/features/explore/Time.tsx b/public/app/features/explore/Time.tsx
new file mode 100644
index 00000000000..01033176f9a
--- /dev/null
+++ b/public/app/features/explore/Time.tsx
@@ -0,0 +1,35 @@
+import React, { FC } from 'react';
+import { toDuration } from '@grafana/data';
+
+export interface TimeProps {
+  timeInMs: number;
+  className?: string;
+  humanize?: boolean;
+}
+
+export const Time: FC<TimeProps> = ({ timeInMs, className, humanize }) => {
+  return <span className={`elapsed-time ${className}`}>{formatTime(timeInMs, humanize)}</span>;
+};
+
+const formatTime = (timeInMs: number, humanize = false): string => {
+  const inSeconds = timeInMs / 1000;
+
+  if (!humanize) {
+    return `${inSeconds.toFixed(1)}s`;
+  }
+
+  const duration = toDuration(inSeconds, 'seconds');
+  const hours = duration.hours();
+  const minutes = duration.minutes();
+  const seconds = duration.seconds();
+
+  if (hours) {
+    return `${hours}h ${minutes}m ${seconds}s`;
+  }
+
+  if (minutes) {
+    return `${minutes}m ${seconds}s`;
+  }
+
+  return `${seconds}s`;
+};
diff --git a/public/app/plugins/datasource/stackdriver/components/Aggregations.test.tsx b/public/app/plugins/datasource/stackdriver/components/Aggregations.test.tsx
index ad1880e9806..06bdd5a29cc 100644
--- a/public/app/plugins/datasource/stackdriver/components/Aggregations.test.tsx
+++ b/public/app/plugins/datasource/stackdriver/components/Aggregations.test.tsx
@@ -1,7 +1,8 @@
 import React from 'react';
 import renderer from 'react-test-renderer';
-import { Aggregations, Props } from './Aggregations';
 import { shallow } from 'enzyme';
+import { Segment } from '@grafana/ui';
+import { Aggregations, Props } from './Aggregations';
 import { ValueTypes, MetricKind } from '../constants';
 import { TemplateSrvStub } from 'test/specs/helpers';
 
@@ -20,39 +21,49 @@ const props: Props = {
 };
 
 describe('Aggregations', () => {
-  let wrapper: any;
   it('renders correctly', () => {
     const tree = renderer.create(<Aggregations {...props} />).toJSON();
     expect(tree).toMatchSnapshot();
   });
 
   describe('options', () => {
-    describe('when DOUBLE and DELTA is passed as props', () => {
-      beforeEach(() => {
-        const newProps = { ...props, metricDescriptor: { valueType: ValueTypes.DOUBLE, metricKind: MetricKind.GAUGE } };
-        wrapper = shallow(<Aggregations {...newProps} />);
-      });
-      it('', () => {
-        const options = wrapper.state().aggOptions;
-        expect(options.length).toEqual(11);
-        expect(options.map((o: any) => o.value)).toEqual(
+    describe('when DOUBLE and GAUGE is passed as props', () => {
+      const nextProps = {
+        ...props,
+        metricDescriptor: {
+          valueType: ValueTypes.DOUBLE,
+          metricKind: MetricKind.GAUGE,
+        },
+      };
+
+      it('should not have the reduce values', () => {
+        const wrapper = shallow(<Aggregations {...nextProps} />);
+        const { options } = wrapper.find(Segment).props() as any;
+        const [, aggGroup] = options;
+
+        expect(aggGroup.options.length).toEqual(11);
+        expect(aggGroup.options.map((o: any) => o.value)).toEqual(
           expect.not.arrayContaining(['REDUCE_COUNT_TRUE', 'REDUCE_COUNT_FALSE'])
         );
       });
     });
 
     describe('when MONEY and CUMULATIVE is passed as props', () => {
-      beforeEach(() => {
-        const newProps = {
-          ...props,
-          metricDescriptor: { valueType: ValueTypes.MONEY, metricKind: MetricKind.CUMULATIVE },
-        };
-        wrapper = shallow(<Aggregations {...newProps} />);
-      });
-      it('', () => {
-        const options = wrapper.state().aggOptions;
-        expect(options.length).toEqual(10);
-        expect(options.map((o: any) => o.value)).toEqual(expect.arrayContaining(['REDUCE_NONE']));
+      const nextProps = {
+        ...props,
+        metricDescriptor: {
+          valueType: ValueTypes.MONEY,
+          metricKind: MetricKind.CUMULATIVE,
+        },
+      };
+
+      it('should have the reduce values', () => {
+        const wrapper = shallow(<Aggregations {...nextProps} />);
+        const { options } = wrapper.find(Segment).props() as any;
+        const [, aggGroup] = options;
+
+        expect(aggGroup.options.length).toEqual(10);
+        expect(aggGroup.options.map((o: any) => o.value)).toEqual(expect.arrayContaining(['REDUCE_NONE']));
       });
     });
   });
diff --git a/public/app/plugins/datasource/stackdriver/components/Aggregations.tsx b/public/app/plugins/datasource/stackdriver/components/Aggregations.tsx
index 10d69c0fba0..326fcd0f20e 100644
--- a/public/app/plugins/datasource/stackdriver/components/Aggregations.tsx
+++ b/public/app/plugins/datasource/stackdriver/components/Aggregations.tsx
@@ -1,4 +1,4 @@
-import React from 'react';
+import React, { FC, useState, useMemo } from 'react';
 import _ from 'lodash';
 
 import { SelectableValue } from '@grafana/data';
@@ -18,81 +18,65 @@ export interface Props {
   templateVariableOptions: Array<SelectableValue<string>>;
 }
 
-export interface State {
-  aggOptions: any[];
-  displayAdvancedOptions: boolean;
-}
+export const Aggregations: FC<Props> = props => {
+  const [displayAdvancedOptions, setDisplayAdvancedOptions] = useState(false);
+  const aggOptions = useAggregationOptionsByMetric(props);
+  const selected = useSelectedFromOptions(aggOptions, props);
 
-export class Aggregations extends React.Component<Props, State> {
-  state: State = {
-    aggOptions: [],
-    displayAdvancedOptions: false,
-  };
-
-  componentDidMount() {
-    this.setAggOptions(this.props);
-  }
-
-  UNSAFE_componentWillReceiveProps(nextProps: Props) {
-    this.setAggOptions(nextProps);
-  }
-
-  setAggOptions({ metricDescriptor }: Props) {
-    let aggOptions: any[] = [];
-    if (metricDescriptor) {
-      aggOptions = getAggregationOptionsByMetric(
-        metricDescriptor.valueType as ValueTypes,
-        metricDescriptor.metricKind as MetricKind
-      ).map(a => ({
-        ...a,
-        label: a.text,
-      }));
-    }
-    this.setState({ aggOptions });
-  }
-
-  onToggleDisplayAdvanced = () => {
-    this.setState(state => ({
-      displayAdvancedOptions: !state.displayAdvancedOptions,
-    }));
-  };
-
-  render() {
-    const { displayAdvancedOptions, aggOptions } = this.state;
-    const { templateVariableOptions, onChange, crossSeriesReducer } = this.props;
-
-    return (
-      <>
-        <div className="gf-form-inline">
-          <label className="gf-form-label query-keyword width-9">Aggregation</label>
-          <Segment
-            onChange={({ value }) => onChange(value)}
-            value={[...aggOptions, ...templateVariableOptions].find(s => s.value === crossSeriesReducer)}
-            options={[
-              {
-                label: 'Template Variables',
-                options: templateVariableOptions,
-              },
-              {
-                label: 'Aggregations',
-                expanded: true,
-                options: aggOptions,
-              },
-            ]}
-            placeholder="Select Reducer"
-          ></Segment>
-          <div className="gf-form gf-form--grow">
-            <label className="gf-form-label gf-form-label--grow">
-              <a onClick={this.onToggleDisplayAdvanced}>
-                <>
-                  <Icon name={displayAdvancedOptions ? 'angle-down' : 'angle-right'} /> Advanced Options
-                </>
-              </a>
-            </label>
-          </div>
+  return (
+    <>
+      <div className="gf-form-inline">
+        <label className="gf-form-label query-keyword width-9">Aggregation</label>
+        <Segment
+          onChange={({ value }) => props.onChange(value)}
+          value={selected}
+          options={[
+            {
+              label: 'Template Variables',
+              options: props.templateVariableOptions,
+            },
+            {
+              label: 'Aggregations',
+              expanded: true,
+              options: aggOptions,
+            },
+          ]}
+          placeholder="Select Reducer"
+        ></Segment>
+        <div className="gf-form gf-form--grow">
+          <label className="gf-form-label gf-form-label--grow">
+            <a onClick={() => setDisplayAdvancedOptions(!displayAdvancedOptions)}>
+              <>
+                <Icon name={displayAdvancedOptions ? 'angle-down' : 'angle-right'} /> Advanced Options
+              </>
+            </a>
+          </label>
         </div>
-        {this.props.children && this.props.children(this.state.displayAdvancedOptions)}
-      </>
-    );
-  }
-}
+      </div>
+      {props.children(displayAdvancedOptions)}
+    </>
+  );
+};
+
+const useAggregationOptionsByMetric = ({ metricDescriptor }: Props): Array<SelectableValue<string>> => {
+  return useMemo(() => {
+    if (!metricDescriptor) {
+      return [];
+    }
+
+    return getAggregationOptionsByMetric(
+      metricDescriptor.valueType as ValueTypes,
+      metricDescriptor.metricKind as MetricKind
+    ).map(a => ({
+      ...a,
+      label: a.text,
+    }));
+  }, [metricDescriptor?.metricKind, metricDescriptor?.valueType]);
+};
+
+const useSelectedFromOptions = (aggOptions: Array<SelectableValue<string>>, props: Props) => {
+  return useMemo(() => {
+    const allOptions = [...aggOptions, ...props.templateVariableOptions];
+    return allOptions.find(s => s.value === props.crossSeriesReducer);
+  }, [aggOptions, props.crossSeriesReducer, props.templateVariableOptions]);
+};