From 583f757d39a47925cbf4d2bda12256a768f366ad Mon Sep 17 00:00:00 2001 From: Marco Liberati Date: Tue, 17 Nov 2020 15:20:02 +0100 Subject: [PATCH] [7.x] [Lens] Avoid unnecessary data fetching on dimension flyout open (#82957) (#83538) Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com> --- .../operations/definitions/helpers.tsx | 30 +++++ .../definitions/ranges/advanced_editor.tsx | 9 +- .../definitions/ranges/range_editor.tsx | 16 +-- .../definitions/ranges/ranges.test.tsx | 111 +++++++++++------- .../terms/values_range_input.test.tsx | 20 ++-- .../definitions/terms/values_range_input.tsx | 6 +- 6 files changed, 129 insertions(+), 63 deletions(-) create mode 100644 x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/helpers.tsx diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/helpers.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/helpers.tsx new file mode 100644 index 0000000000000..a5c08a93467af --- /dev/null +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/helpers.tsx @@ -0,0 +1,30 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ + +import { useRef } from 'react'; +import useDebounce from 'react-use/lib/useDebounce'; + +export const useDebounceWithOptions = ( + fn: Function, + { skipFirstRender }: { skipFirstRender: boolean } = { skipFirstRender: false }, + ms?: number | undefined, + deps?: React.DependencyList | undefined +) => { + const isFirstRender = useRef(true); + const newDeps = [...(deps || []), isFirstRender]; + + return useDebounce( + () => { + if (skipFirstRender && isFirstRender.current) { + isFirstRender.current = false; + return; + } + return fn(); + }, + ms, + newDeps + ); +}; diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx index 2eb971aa03c55..95c7e3533ee09 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx @@ -8,7 +8,6 @@ import './advanced_editor.scss'; import React, { useState, MouseEventHandler } from 'react'; import { i18n } from '@kbn/i18n'; -import useDebounce from 'react-use/lib/useDebounce'; import { EuiFlexGroup, EuiFlexItem, @@ -31,6 +30,7 @@ import { DraggableBucketContainer, LabelInput, } from '../shared_components'; +import { useDebounceWithOptions } from '../helpers'; const generateId = htmlIdGenerator(); @@ -208,12 +208,13 @@ export const AdvancedRangeEditor = ({ const lastIndex = localRanges.length - 1; - // Update locally all the time, but bounce the parents prop function - // to aviod too many requests - useDebounce( + // Update locally all the time, but bounce the parents prop function to aviod too many requests + // Avoid to trigger on first render + useDebounceWithOptions( () => { setRanges(localRanges.map(({ id, ...rest }) => ({ ...rest }))); }, + { skipFirstRender: true }, TYPING_DEBOUNCE_TIME, [localRanges] ); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/range_editor.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/range_editor.tsx index a18c47f9dedd1..df955be6b490a 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/range_editor.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/range_editor.tsx @@ -6,7 +6,6 @@ import React, { useEffect, useState } from 'react'; import { i18n } from '@kbn/i18n'; -import useDebounce from 'react-use/lib/useDebounce'; import { EuiButtonEmpty, EuiFormRow, @@ -21,6 +20,7 @@ import { IFieldFormat } from 'src/plugins/data/public'; import { RangeColumnParams, UpdateParamsFnType, MODES_TYPES } from './ranges'; import { AdvancedRangeEditor } from './advanced_editor'; import { TYPING_DEBOUNCE_TIME, MODES, MIN_HISTOGRAM_BARS } from './constants'; +import { useDebounceWithOptions } from '../helpers'; const BaseRangeEditor = ({ maxBars, @@ -37,10 +37,11 @@ const BaseRangeEditor = ({ }) => { const [maxBarsValue, setMaxBarsValue] = useState(String(maxBars)); - useDebounce( + useDebounceWithOptions( () => { onMaxBarsChange(Number(maxBarsValue)); }, + { skipFirstRender: true }, TYPING_DEBOUNCE_TIME, [maxBarsValue] ); @@ -151,13 +152,14 @@ export const RangeEditor = ({ }) => { const [isAdvancedEditor, toggleAdvancedEditor] = useState(params.type === MODES.Range); - // if the maxBars in the params is set to auto refresh it with the default value - // only on bootstrap + // if the maxBars in the params is set to auto refresh it with the default value only on bootstrap useEffect(() => { - if (params.maxBars !== maxBars) { - setParam('maxBars', maxBars); + if (!isAdvancedEditor) { + if (params.maxBars !== maxBars) { + setParam('maxBars', maxBars); + } } - }, [maxBars, params.maxBars, setParam]); + }, [maxBars, params.maxBars, setParam, isAdvancedEditor]); if (isAdvancedEditor) { return ( diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/ranges.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/ranges.test.tsx index ce015284e544b..87dcdb45cf58f 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/ranges.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/ranges.test.tsx @@ -284,7 +284,11 @@ describe('ranges', () => { /> ); + // There's a useEffect in the component that updates the value on bootstrap + // because there's a debouncer, wait a bit before calling onChange act(() => { + jest.advanceTimersByTime(TYPING_DEBOUNCE_TIME * 4); + instance.find(EuiRange).prop('onChange')!( { currentTarget: { @@ -293,26 +297,27 @@ describe('ranges', () => { } as React.ChangeEvent, true ); + jest.advanceTimersByTime(TYPING_DEBOUNCE_TIME * 4); + }); - expect(setStateSpy).toHaveBeenCalledWith({ - ...state, - layers: { - first: { - ...state.layers.first, - columns: { - ...state.layers.first.columns, - col1: { - ...state.layers.first.columns.col1, - params: { - ...state.layers.first.columns.col1.params, - maxBars: MAX_HISTOGRAM_VALUE, - }, + expect(setStateSpy).toHaveBeenCalledWith({ + ...state, + layers: { + first: { + ...state.layers.first, + columns: { + ...state.layers.first.columns, + col1: { + ...state.layers.first.columns.col1, + params: { + ...state.layers.first.columns.col1.params, + maxBars: MAX_HISTOGRAM_VALUE, }, }, }, }, - }); + }, }); }); @@ -330,59 +335,65 @@ describe('ranges', () => { /> ); + // There's a useEffect in the component that updates the value on bootstrap + // because there's a debouncer, wait a bit before calling onChange act(() => { + jest.advanceTimersByTime(TYPING_DEBOUNCE_TIME * 4); // minus button instance .find('[data-test-subj="lns-indexPattern-range-maxBars-minus"]') .find('button') .prop('onClick')!({} as ReactMouseEvent); jest.advanceTimersByTime(TYPING_DEBOUNCE_TIME * 4); + }); - expect(setStateSpy).toHaveBeenCalledWith({ - ...state, - layers: { - first: { - ...state.layers.first, - columns: { - ...state.layers.first.columns, - col1: { - ...state.layers.first.columns.col1, - params: { - ...state.layers.first.columns.col1.params, - maxBars: GRANULARITY_DEFAULT_VALUE - GRANULARITY_STEP, - }, + expect(setStateSpy).toHaveBeenCalledWith({ + ...state, + layers: { + first: { + ...state.layers.first, + columns: { + ...state.layers.first.columns, + col1: { + ...state.layers.first.columns.col1, + params: { + ...state.layers.first.columns.col1.params, + maxBars: GRANULARITY_DEFAULT_VALUE - GRANULARITY_STEP, }, }, }, }, - }); + }, + }); + act(() => { // plus button instance .find('[data-test-subj="lns-indexPattern-range-maxBars-plus"]') .find('button') .prop('onClick')!({} as ReactMouseEvent); jest.advanceTimersByTime(TYPING_DEBOUNCE_TIME * 4); + }); - expect(setStateSpy).toHaveBeenCalledWith({ - ...state, - layers: { - first: { - ...state.layers.first, - columns: { - ...state.layers.first.columns, - col1: { - ...state.layers.first.columns.col1, - params: { - ...state.layers.first.columns.col1.params, - maxBars: GRANULARITY_DEFAULT_VALUE, - }, + expect(setStateSpy).toHaveBeenCalledWith({ + ...state, + layers: { + first: { + ...state.layers.first, + columns: { + ...state.layers.first.columns, + col1: { + ...state.layers.first.columns.col1, + params: { + ...state.layers.first.columns.col1.params, + maxBars: GRANULARITY_DEFAULT_VALUE, }, }, }, }, - }); + }, }); + // }); }); }); @@ -749,6 +760,22 @@ describe('ranges', () => { ); }); + it('should not update the state on mount', () => { + const setStateSpy = jest.fn(); + + mount( + + ); + expect(setStateSpy.mock.calls.length).toBe(0); + }); + it('should not reset formatters when switching between custom ranges and auto histogram', () => { const setStateSpy = jest.fn(); // now set a format on the range operation diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.test.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.test.tsx index 18b9b5b1e8b98..759bda43efe67 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.test.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.test.tsx @@ -20,6 +20,13 @@ describe('ValuesRangeInput', () => { expect(instance.find(EuiRange).prop('value')).toEqual('5'); }); + it('should not run onChange function on mount', () => { + const onChangeSpy = jest.fn(); + shallow(); + + expect(onChangeSpy.mock.calls.length).toBe(0); + }); + it('should run onChange function on update', () => { const onChangeSpy = jest.fn(); const instance = shallow(); @@ -30,11 +37,10 @@ describe('ValuesRangeInput', () => { ); }); expect(instance.find(EuiRange).prop('value')).toEqual('7'); - // useDebounce runs on initialization and on change - expect(onChangeSpy.mock.calls.length).toBe(2); - expect(onChangeSpy.mock.calls[0][0]).toBe(5); - expect(onChangeSpy.mock.calls[1][0]).toBe(7); + expect(onChangeSpy.mock.calls.length).toBe(1); + expect(onChangeSpy.mock.calls[0][0]).toBe(7); }); + it('should not run onChange function on update when value is out of 1-100 range', () => { const onChangeSpy = jest.fn(); const instance = shallow(); @@ -46,9 +52,7 @@ describe('ValuesRangeInput', () => { }); instance.update(); expect(instance.find(EuiRange).prop('value')).toEqual('107'); - // useDebounce only runs on initialization - expect(onChangeSpy.mock.calls.length).toBe(2); - expect(onChangeSpy.mock.calls[0][0]).toBe(5); - expect(onChangeSpy.mock.calls[1][0]).toBe(100); + expect(onChangeSpy.mock.calls.length).toBe(1); + expect(onChangeSpy.mock.calls[0][0]).toBe(100); }); }); diff --git a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx index ef42f2d4a7175..7018ba3083f04 100644 --- a/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx +++ b/x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx @@ -5,9 +5,9 @@ */ import React, { useState } from 'react'; -import useDebounce from 'react-use/lib/useDebounce'; import { i18n } from '@kbn/i18n'; import { EuiRange } from '@elastic/eui'; +import { useDebounceWithOptions } from '../helpers'; export const ValuesRangeInput = ({ value, @@ -20,7 +20,8 @@ export const ValuesRangeInput = ({ const MAX_NUMBER_OF_VALUES = 100; const [inputValue, setInputValue] = useState(String(value)); - useDebounce( + + useDebounceWithOptions( () => { if (inputValue === '') { return; @@ -28,6 +29,7 @@ export const ValuesRangeInput = ({ const inputNumber = Number(inputValue); onChange(Math.min(MAX_NUMBER_OF_VALUES, Math.max(inputNumber, MIN_NUMBER_OF_VALUES))); }, + { skipFirstRender: true }, 256, [inputValue] );