From c600a08524566b9a708266a30a4cc2367c92b893 Mon Sep 17 00:00:00 2001 From: Boyko Date: Fri, 20 Mar 2020 14:00:49 +0200 Subject: [PATCH] Query components unsafe lifecycle methods (#21163) * Chore: refactor query components unsafe lifecycle methods * Chore: remove redundant defaults * Chore: check expression exist while filter targets in query * Chore: fix broken test * Chore: adding query filed tests * pass normalized query to QueryFieldsEditor Signed-off-by: Boyko Lalov * Remove introduced strictNullCheck errors and correctly solve merged conficts * Improve redability and fix strictNullChecks Co-authored-by: Ivana --- .../components/QueryField/QueryField.test.tsx | 32 +++++++ .../src/components/QueryField/QueryField.tsx | 20 ++--- .../components/QueryEditor.test.tsx | 35 ++++---- .../cloudwatch/components/QueryEditor.tsx | 83 ++++++++----------- .../datasource/cloudwatch/datasource.ts | 6 +- 5 files changed, 91 insertions(+), 85 deletions(-) diff --git a/packages/grafana-ui/src/components/QueryField/QueryField.test.tsx b/packages/grafana-ui/src/components/QueryField/QueryField.test.tsx index 1a453f1f06c..b124d81ed35 100644 --- a/packages/grafana-ui/src/components/QueryField/QueryField.test.tsx +++ b/packages/grafana-ui/src/components/QueryField/QueryField.test.tsx @@ -49,4 +49,36 @@ describe('', () => { expect(onBlur.mock.calls.length).toBe(1); expect(onRun.mock.calls.length).toBe(0); }); + describe('syntaxLoaded', () => { + it('should re-render the editor after syntax has fully loaded', () => { + const wrapper: any = shallow(); + const spyOnChange = jest.spyOn(wrapper.instance(), 'onChange').mockImplementation(jest.fn()); + wrapper.instance().editor = { insertText: () => ({ deleteBackward: () => ({ value: 'fooo' }) }) }; + wrapper.setProps({ syntaxLoaded: true }); + expect(spyOnChange).toHaveBeenCalledWith('fooo', true); + }); + it('should not re-render the editor if syntax is already loaded', () => { + const wrapper: any = shallow(); + const spyOnChange = jest.spyOn(wrapper.instance(), 'onChange').mockImplementation(jest.fn()); + wrapper.setProps({ syntaxLoaded: true }); + wrapper.instance().editor = {}; + wrapper.setProps({ syntaxLoaded: true }); + expect(spyOnChange).not.toBeCalled(); + }); + it('should not re-render the editor if editor itself is not defined', () => { + const wrapper: any = shallow(); + const spyOnChange = jest.spyOn(wrapper.instance(), 'onChange').mockImplementation(jest.fn()); + wrapper.setProps({ syntaxLoaded: true }); + expect(wrapper.instance().editor).toBeFalsy(); + expect(spyOnChange).not.toBeCalled(); + }); + it('should not re-render the editor twice once syntax is fully lodaded', () => { + const wrapper: any = shallow(); + const spyOnChange = jest.spyOn(wrapper.instance(), 'onChange').mockImplementation(jest.fn()); + wrapper.instance().editor = { insertText: () => ({ deleteBackward: () => ({ value: 'fooo' }) }) }; + wrapper.setProps({ syntaxLoaded: true }); + wrapper.setProps({ syntaxLoaded: true }); + expect(spyOnChange).toBeCalledTimes(1); + }); + }); }); diff --git a/packages/grafana-ui/src/components/QueryField/QueryField.tsx b/packages/grafana-ui/src/components/QueryField/QueryField.tsx index 7bfe98e84aa..0abacd390fe 100644 --- a/packages/grafana-ui/src/components/QueryField/QueryField.tsx +++ b/packages/grafana-ui/src/components/QueryField/QueryField.tsx @@ -95,7 +95,13 @@ export class QueryField extends React.PureComponent { @@ -86,26 +86,19 @@ describe('QueryEditor', () => { }); }); - it('should init props correctly', async () => { - // @ts-ignore strict null error TS2345: Argument of type '() => Promise' is not assignable to parameter of type '() => void | undefined'. - await act(async () => { - const props = setup(); - props.query.namespace = (null as unknown) as string; - props.query.metricName = (null as unknown) as string; - props.query.expression = (null as unknown) as string; - props.query.dimensions = (null as unknown) as { [key: string]: string | string[] }; - props.query.region = (null as unknown) as string; - props.query.statistics = (null as unknown) as string[]; - const wrapper = mount(); - const { - query: { namespace, region, metricName, dimensions, statistics, expression }, - } = wrapper.props(); - expect(namespace).toEqual(''); - expect(metricName).toEqual(''); - expect(expression).toEqual(''); - expect(region).toEqual('default'); - expect(statistics).toEqual(['Average']); - expect(dimensions).toEqual({}); + it('should normalize query with default values', () => { + expect(normalizeQuery({ refId: '42' } as any)).toEqual({ + namespace: '', + metricName: '', + expression: '', + dimensions: {}, + region: 'default', + id: '', + alias: '', + statistics: ['Average'], + matchExact: true, + period: '', + refId: '42', }); }); }); diff --git a/public/app/plugins/datasource/cloudwatch/components/QueryEditor.tsx b/public/app/plugins/datasource/cloudwatch/components/QueryEditor.tsx index 2e5873cd9bb..aeb4b1cad17 100644 --- a/public/app/plugins/datasource/cloudwatch/components/QueryEditor.tsx +++ b/public/app/plugins/datasource/cloudwatch/components/QueryEditor.tsx @@ -1,6 +1,7 @@ import React, { PureComponent, ChangeEvent } from 'react'; import { ExploreQueryFieldProps } from '@grafana/data'; import { Input, ValidationEvents, EventsWithValidation, Switch } from '@grafana/ui'; +import isEmpty from 'lodash/isEmpty'; import { CloudWatchQuery } from '../types'; import CloudWatchDatasource from '../datasource'; import { QueryField, Alias, QueryFieldsEditor } from './'; @@ -20,51 +21,36 @@ const idValidationEvents: ValidationEvents = { ], }; +export const normalizeQuery = ({ + namespace, + metricName, + expression, + dimensions, + region, + id, + alias, + statistics, + period, + ...rest +}: CloudWatchQuery): CloudWatchQuery => { + const normalizedQuery = { + namespace: namespace || '', + metricName: metricName || '', + expression: expression || '', + dimensions: dimensions || {}, + region: region || 'default', + id: id || '', + alias: alias || '', + statistics: isEmpty(statistics) ? ['Average'] : statistics, + period: period || '', + ...rest, + }; + return !rest.hasOwnProperty('matchExact') ? { ...normalizedQuery, matchExact: true } : normalizedQuery; +}; + export class QueryEditor extends PureComponent { state: State = { showMeta: false }; - static getDerivedStateFromProps(props: Props, state: State) { - const { query } = props; - - if (!query.namespace) { - query.namespace = ''; - } - - if (!query.metricName) { - query.metricName = ''; - } - - if (!query.expression) { - query.expression = ''; - } - - if (!query.dimensions) { - query.dimensions = {}; - } - - if (!query.region) { - query.region = 'default'; - } - - if (!query.id) { - query.id = ''; - } - - if (!query.alias) { - query.alias = ''; - } - - if (!query.statistics || !query.statistics.length) { - query.statistics = ['Average']; - } - - if (!query.hasOwnProperty('matchExact')) { - query.matchExact = true; - } - - return state; - } - onChange(query: CloudWatchQuery) { const { onChange, onRunQuery } = this.props; onChange(query); @@ -72,12 +58,13 @@ export class QueryEditor extends PureComponent { } render() { - const { data, query, onRunQuery } = this.props; + const { data, onRunQuery } = this.props; const { showMeta } = this.state; + const query = normalizeQuery(this.props.query); const metaDataExist = data && Object.values(data).length && data.state === 'Done'; return ( <> - + {query.statistics.length <= 1 && (
@@ -92,7 +79,7 @@ export class QueryEditor extends PureComponent { this.onChange({ ...query, id: event.target.value }) } validationEvents={idValidationEvents} - value={query.id || ''} + value={query.id} />
@@ -105,7 +92,7 @@ export class QueryEditor extends PureComponent { ) => this.onChange({ ...query, expression: event.target.value }) } @@ -119,7 +106,7 @@ export class QueryEditor extends PureComponent { ) => @@ -170,7 +157,7 @@ export class QueryEditor extends PureComponent { - {data.series[0].meta.gmdMeta.map(({ ID, Expression, Period }: any) => ( + {data?.series[0]?.meta?.gmdMeta.map(({ ID, Expression, Period }: any) => ( {ID} {Expression} diff --git a/public/app/plugins/datasource/cloudwatch/datasource.ts b/public/app/plugins/datasource/cloudwatch/datasource.ts index 23dadaecaa7..16ab6851059 100644 --- a/public/app/plugins/datasource/cloudwatch/datasource.ts +++ b/public/app/plugins/datasource/cloudwatch/datasource.ts @@ -68,7 +68,7 @@ export default class CloudWatchDatasource extends DataSourceApi 0) + item.expression?.length > 0) ); }).map(item => { item.region = this.replace(this.getActualRegion(item.region), options.scopedVars, true, 'region'); @@ -112,8 +112,8 @@ export default class CloudWatchDatasource extends DataSourceApi