Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[7.x] [Lens] Avoid unnecessary data fetching on dimension flyout open (#82957) #83538

Merged
merged 1 commit into from
Nov 17, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
);
};
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
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
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
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);
});
});
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