Skip to content

Commit

Permalink
[Lens] Avoid unnecessary data fetching on dimension flyout open (#82957)
Browse files Browse the repository at this point in the history 
Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
  • Loading branch information
@dej611 @kibanamachine
dej611 and kibanamachine authored Nov 16, 2020
1 parent 01b1710 commit c8b8a0a
Show file tree
Hide file tree
Showing 6 changed files with 129 additions and 63 deletions.
30 changes: 30 additions & 0 deletions x-pack/plugins/lens/public/indexpattern_datasource/operations/definitions/helpers.tsx
View file
Original file line number Diff line number Diff line change
@@ -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
);
};
9 changes: 5 additions & 4 deletions ...ins/lens/public/indexpattern_datasource/operations/definitions/ranges/advanced_editor.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -31,6 +30,7 @@ import {
DraggableBucketContainer,
LabelInput,
} from '../shared_components';
import { useDebounceWithOptions } from '../helpers';

const generateId = htmlIdGenerator();

Expand Down Expand Up @@ -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]
);
Expand Down
16 changes: 9 additions & 7 deletions ...lugins/lens/public/indexpattern_datasource/operations/definitions/ranges/range_editor.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -37,10 +37,11 @@ const BaseRangeEditor = ({
}) => {
const [maxBarsValue, setMaxBarsValue] = useState(String(maxBars));

useDebounce(
useDebounceWithOptions(
() => {
onMaxBarsChange(Number(maxBarsValue));
},
{ skipFirstRender: true },
TYPING_DEBOUNCE_TIME,
[maxBarsValue]
);
Expand Down Expand Up @@ -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 (
Expand Down
111 changes: 69 additions & 42 deletions ...plugins/lens/public/indexpattern_datasource/operations/definitions/ranges/ranges.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: {
Expand All @@ -293,26 +297,27 @@ describe('ranges', () => {
} as React.ChangeEvent<HTMLInputElement>,
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,
},
},
},
},
});
},
});
});

Expand All @@ -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,
},
},
},
},
});
},
});
// });
});
});

Expand Down Expand Up @@ -749,6 +760,22 @@ describe('ranges', () => {
);
});

it('should not update the state on mount', () => {
const setStateSpy = jest.fn();

mount(
<InlineOptions
{...defaultOptions}
state={state}
setState={setStateSpy}
columnId="col1"
currentColumn={state.layers.first.columns.col1 as RangeIndexPatternColumn}
layerId="first"
/>
);
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
Expand Down
20 changes: 12 additions & 8 deletions ...s/public/indexpattern_datasource/operations/definitions/terms/values_range_input.test.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(<ValuesRangeInput value={5} onChange={onChangeSpy} />);

expect(onChangeSpy.mock.calls.length).toBe(0);
});

it('should run onChange function on update', () => {
const onChangeSpy = jest.fn();
const instance = shallow(<ValuesRangeInput value={5} onChange={onChangeSpy} />);
Expand All @@ -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(<ValuesRangeInput value={5} onChange={onChangeSpy} />);
Expand All @@ -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);
});
});
6 changes: 4 additions & 2 deletions ...s/lens/public/indexpattern_datasource/operations/definitions/terms/values_range_input.tsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -20,14 +20,16 @@ export const ValuesRangeInput = ({
const MAX_NUMBER_OF_VALUES = 100;

const [inputValue, setInputValue] = useState(String(value));
useDebounce(

useDebounceWithOptions(
() => {
if (inputValue === '') {
return;
}
const inputNumber = Number(inputValue);
onChange(Math.min(MAX_NUMBER_OF_VALUES, Math.max(inputNumber, MIN_NUMBER_OF_VALUES)));
},
{ skipFirstRender: true },
256,
[inputValue]
);
Expand Down

0 comments on commit c8b8a0a

Please sign in to comment.