From caa788dc5d2a8748cfdc54b1c437a004689205cd Mon Sep 17 00:00:00 2001 From: Kamil Gabryjelski Date: Thu, 14 Oct 2021 09:14:44 +0200 Subject: [PATCH 1/3] perf(native-filters): decrease number of redundant rerenders --- .../FilterControls/FilterControl.tsx | 23 +-- .../FilterControls/FilterControls.tsx | 19 ++- .../FilterBar/FilterControls/FilterValue.tsx | 37 ++++- .../FilterBar/FilterControls/state.ts | 23 +-- .../nativeFilters/FilterBar/index.tsx | 40 +++-- .../nativeFilters/FilterBar/state.ts | 59 +++++--- .../components/Range/RangeFilterPlugin.tsx | 142 ++++++++++-------- .../components/Select/SelectFilterPlugin.tsx | 61 ++++---- .../components/Time/TimeFilterPlugin.tsx | 40 +++-- 9 files changed, 261 insertions(+), 183 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx index c952c1f25067..a65b93972f17 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React from 'react'; +import React, { useMemo } from 'react'; import { styled } from '@superset-ui/core'; import { Form, FormItem } from 'src/components/Form'; import FilterValue from './FilterValue'; @@ -67,17 +67,22 @@ const FilterControl: React.FC = ({ filter.dataMask?.filterState, ); + const label = useMemo( + () => ( + + + {name} + + {icon} + + ), + [icon, name], + ); + return ( - - {name} - - {icon} - - } + label={label} required={filter?.controlValues?.enableEmptyFilter} validateStatus={isMissingRequiredValue ? 'error' : undefined} > diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx index 613bc4ab89cf..1c98ac53eff7 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx @@ -16,7 +16,7 @@ * specific language governing permissions and limitations * under the License. */ -import React, { FC, useMemo, useState } from 'react'; +import React, { FC, useCallback, useMemo, useState } from 'react'; import { css } from '@emotion/react'; import { DataMask, styled, t } from '@superset-ui/core'; import { @@ -55,8 +55,8 @@ const FilterControls: FC = ({ }) => { const [visiblePopoverId, setVisiblePopoverId] = useState(null); const filters = useFilters(); - const filterValues = Object.values(filters); - const portalNodes = React.useMemo(() => { + const filterValues = useMemo(() => Object.values(filters), [filters]); + const portalNodes = useMemo(() => { const nodes = new Array(filterValues.length); for (let i = 0; i < filterValues.length; i += 1) { nodes[i] = createHtmlPortalNode(); @@ -70,10 +70,15 @@ const FilterControls: FC = ({ dataMask: dataMaskSelected[filter.id], })); return buildCascadeFiltersTree(filtersWithValue); - // eslint-disable-next-line react-hooks/exhaustive-deps - }, [JSON.stringify(filterValues), dataMaskSelected]); + }, [filterValues, dataMaskSelected]); const cascadeFilterIds = new Set(cascadeFilters.map(item => item.id)); + const handleVisibleChange = useCallback( + index => (visible: boolean) => + setVisiblePopoverId(visible ? cascadeFilters[index].id : null), + [cascadeFilters], + ); + const [filtersInScope, filtersOutOfScope] = useSelectFiltersInScope( cascadeFilters, ); @@ -91,9 +96,7 @@ const FilterControls: FC = ({ key={cascadeFilters[index].id} dataMaskSelected={dataMaskSelected} visible={visiblePopoverId === cascadeFilters[index].id} - onVisibleChange={visible => - setVisiblePopoverId(visible ? cascadeFilters[index].id : null) - } + onVisibleChange={handleVisibleChange(index)} filter={cascadeFilters[index]} onFilterSelectionChange={onFilterSelectionChange} directPathToChild={directPathToChild} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx index dd0c56c864aa..4ea8b5fc4d5a 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx @@ -16,7 +16,13 @@ * specific language governing permissions and limitations * under the License. */ -import React, { useEffect, useRef, useState } from 'react'; +import React, { + useCallback, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { QueryFormData, SuperChart, @@ -53,6 +59,9 @@ const StyledDiv = styled.div` } `; +const queriesDataPlaceholder = [{ data: [{}] }]; +const behaviors = [Behavior.NATIVE_FILTER]; + const FilterValue: React.FC = ({ dataMaskSelected, filter, @@ -193,11 +202,23 @@ const FilterValue: React.FC = ({ return undefined; }, [inputRef, directPathToChild, filter.id]); - const setDataMask = (dataMask: DataMask) => - onFilterSelectionChange(filter, dataMask); + const setDataMask = useCallback( + (dataMask: DataMask) => onFilterSelectionChange(filter, dataMask), + [filter, onFilterSelectionChange], + ); + + const setFocusedFilter = useCallback( + () => dispatchFocusAction(dispatch, id), + [dispatch, id], + ); + const unsetFocusedFilter = useCallback(() => dispatchFocusAction(dispatch), [ + dispatch, + ]); - const setFocusedFilter = () => dispatchFocusAction(dispatch, id); - const unsetFocusedFilter = () => dispatchFocusAction(dispatch); + const hooks = useMemo( + () => ({ setDataMask, setFocusedFilter, unsetFocusedFilter }), + [setDataMask, setFocusedFilter, unsetFocusedFilter], + ); if (error) { return ( @@ -227,14 +248,14 @@ const FilterValue: React.FC = ({ width="100%" formData={formData} // For charts that don't have datasource we need workaround for empty placeholder - queriesData={hasDataSource ? state : [{ data: [{}] }]} + queriesData={hasDataSource ? state : queriesDataPlaceholder} chartType={filterType} - behaviors={[Behavior.NATIVE_FILTER]} + behaviors={behaviors} filterState={filterState} ownState={filter.dataMask?.ownState} enableNoResults={metadata?.enableNoResults} isRefreshing={isRefreshing} - hooks={{ setDataMask, setFocusedFilter, unsetFocusedFilter }} + hooks={hooks} /> )} diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/state.ts b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/state.ts index 1aaf772722c1..810ef5d8b488 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/state.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/state.ts @@ -16,6 +16,7 @@ * specific language governing permissions and limitations * under the License. */ +import { useMemo } from 'react'; import { useSelector } from 'react-redux'; import { NativeFiltersState } from 'src/dashboard/reducers/types'; import { DataMaskStateWithId } from 'src/dataMask/types'; @@ -31,14 +32,16 @@ export function useCascadingFilters( state => state.nativeFilters, ); const filter = filters[id]; - const cascadeParentIds: string[] = filter?.cascadeParentIds ?? []; - let cascadedFilters = {}; - cascadeParentIds.forEach(parentId => { - const parentState = dataMaskSelected?.[parentId]; - cascadedFilters = mergeExtraFormData( - cascadedFilters, - parentState?.extraFormData, - ); - }); - return cascadedFilters; + return useMemo(() => { + const cascadeParentIds: string[] = filter?.cascadeParentIds ?? []; + let cascadedFilters = {}; + cascadeParentIds.forEach(parentId => { + const parentState = dataMaskSelected?.[parentId]; + cascadedFilters = mergeExtraFormData( + cascadedFilters, + parentState?.extraFormData, + ); + }); + return cascadedFilters; + }, [dataMaskSelected, filter?.cascadeParentIds]); } diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx index b0499eb4552b..29f6224ec146 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FilterBar/index.tsx @@ -19,7 +19,7 @@ /* eslint-disable no-param-reassign */ import { DataMask, HandlerFunction, styled, t } from '@superset-ui/core'; -import React, { useEffect, useState, useCallback } from 'react'; +import React, { useEffect, useState, useCallback, useMemo } from 'react'; import { useDispatch } from 'react-redux'; import cx from 'classnames'; import Icons from 'src/components/Icons'; @@ -162,7 +162,7 @@ const FilterBar: React.FC = ({ const previousFilters = usePrevious(filters); const filterValues = Object.values(filters); - const handleFilterSelectionChange = ( + const handleFilterSelectionChange = useCallback(( filter: Pick & Partial, dataMask: Partial, ) => { @@ -177,12 +177,14 @@ const FilterBar: React.FC = ({ dispatch(updateDataMask(filter.id, dataMask)); } - draft[filter.id] = { - ...(getInitialDataMask(filter.id) as DataMaskWithId), - ...dataMask, - }; - }); - }; + draft[filter.id] = { + ...(getInitialDataMask(filter.id) as DataMaskWithId), + ...dataMask, + }; + }); + }, + [dataMaskSelected, dispatch, setDataMaskSelected, tab] + ); const publishDataMask = useCallback( (dataMaskSelected: DataMaskStateWithId) => { @@ -246,23 +248,27 @@ const FilterBar: React.FC = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, [dataMaskAppliedText, publishDataMask]); - const handleApply = () => { + const handleApply = useCallback(() => { const filterIds = Object.keys(dataMaskSelected); filterIds.forEach(filterId => { if (dataMaskSelected[filterId]) { dispatch(updateDataMask(filterId, dataMaskSelected[filterId])); } }); - }; + }, [dataMaskSelected, dispatch]); - const handleClearAll = () => { + const handleClearAll = useCallback(() => { const filterIds = Object.keys(dataMaskSelected); filterIds.forEach(filterId => { if (dataMaskSelected[filterId]) { dispatch(clearDataMask(filterId)); } }); - }; + }, [dataMaskSelected, dispatch]); + + const openFiltersBar = useCallback(() => toggleFiltersBar(true), [ + toggleFiltersBar, + ]); useFilterUpdates(dataMaskSelected, setDataMaskSelected); const isApplyDisabled = checkIsApplyDisabled( @@ -272,6 +278,8 @@ const FilterBar: React.FC = ({ ); const isInitialized = useInitialization(); + const tabPaneStyle = useMemo(() => ({ overflow: 'auto', height }), [height]); + return ( = ({ toggleFiltersBar(true)} + onClick={openFiltersBar} offset={offset} > = ({ {editFilterSetId && ( = ({ disabled={!!editFilterSetId} tab={t(`Filter Sets (${filterSetFilterValues.length})`)} key={TabIds.FilterSets} - css={{ overflow: 'auto', height }} + css={tabPaneStyle} > = ({ ) : ( -
+
state => state.nativeFilters.filterSets || {}, ); -export const useFilters = () => - useSelector(state => { - const preselectNativeFilters = - state.dashboardState?.preselectNativeFilters || {}; - return Object.entries(state.nativeFilters.filters).reduce( - (acc, [filterId, filter]: [string, Filter]) => ({ - ...acc, - [filterId]: { - ...filter, - preselect: preselectNativeFilters[filterId], - }, - }), - {} as Filters, - ); - }); +export const useFilters = () => { + const preselectedNativeFilters = useSelector( + state => state.dashboardState?.preselectNativeFilters, + ); + const nativeFilters = useSelector( + state => state.nativeFilters.filters, + ); + return useMemo( + () => + Object.entries(nativeFilters).reduce( + (acc, [filterId, filter]: [string, Filter]) => ({ + ...acc, + [filterId]: { + ...filter, + preselect: preselectedNativeFilters?.[filterId], + }, + }), + {} as Filters, + ), + [nativeFilters, preselectedNativeFilters], + ); +}; export const useNativeFiltersDataMask = () => { const dataMask = useSelector( state => state.dataMask, ); - return Object.values(dataMask) - .filter((item: DataMaskWithId) => - String(item.id).startsWith(NATIVE_FILTER_PREFIX), - ) - .reduce( - (prev, next: DataMaskWithId) => ({ ...prev, [next.id]: next }), - {}, - ) as DataMaskStateWithId; + return useMemo( + () => + Object.values(dataMask) + .filter((item: DataMaskWithId) => + String(item.id).startsWith(NATIVE_FILTER_PREFIX), + ) + .reduce( + (prev, next: DataMaskWithId) => ({ ...prev, [next.id]: next }), + {}, + ) as DataMaskStateWithId, + [dataMask], + ); }; export const useFilterUpdates = ( diff --git a/superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx b/superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx index 2cd8129edcf1..d83465fbd626 100644 --- a/superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx +++ b/superset-frontend/src/filters/components/Range/RangeFilterPlugin.tsx @@ -22,10 +22,9 @@ import { styled, t, } from '@superset-ui/core'; -import React, { useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useMemo, useState } from 'react'; import { Slider } from 'src/common/components'; import { rgba } from 'emotion-rgba'; -import { FormItemProps } from 'antd/lib/form'; import { PluginFilterRangeProps } from './types'; import { StatusMessage, StyledFormItem, FilterPluginStyle } from '../common'; import { getRangeExtraFormData } from '../../utils'; @@ -74,6 +73,37 @@ const Wrapper = styled.div<{ validateStatus?: 'error' | 'warning' | 'info' }>` `} `; +const numberFormatter = getNumberFormatter(NumberFormats.SMART_NUMBER); + +const tipFormatter = (value: number) => numberFormatter(value); + +const getLabel = (lower: number | null, upper: number | null): string => { + if (lower !== null && upper !== null) { + return `${numberFormatter(lower)} ≤ x ≤ ${numberFormatter(upper)}`; + } + if (lower !== null) { + return `x ≥ ${numberFormatter(lower)}`; + } + if (upper !== null) { + return `x ≤ ${numberFormatter(upper)}`; + } + return ''; +}; + +const getMarks = ( + lower: number | null, + upper: number | null, +): { [key: number]: string } => { + const newMarks: { [key: number]: string } = {}; + if (lower !== null) { + newMarks[lower] = numberFormatter(lower); + } + if (upper !== null) { + newMarks[upper] = numberFormatter(upper); + } + return newMarks; +}; + export default function RangeFilterPlugin(props: PluginFilterRangeProps) { const { data, @@ -85,8 +115,6 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) { unsetFocusedFilter, filterState, } = props; - const numberFormatter = getNumberFormatter(NumberFormats.SMART_NUMBER); - const [row] = data; // @ts-ignore const { min, max }: { min: number; max: number } = row; @@ -97,60 +125,39 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) { ); const [marks, setMarks] = useState<{ [key: number]: string }>({}); - const getBounds = ( - value: [number, number], - ): { lower: number | null; upper: number | null } => { - const [lowerRaw, upperRaw] = value; - return { - lower: lowerRaw > min ? lowerRaw : null, - upper: upperRaw < max ? upperRaw : null, - }; - }; - - const getLabel = (lower: number | null, upper: number | null): string => { - if (lower !== null && upper !== null) { - return `${numberFormatter(lower)} ≤ x ≤ ${numberFormatter(upper)}`; - } - if (lower !== null) { - return `x ≥ ${numberFormatter(lower)}`; - } - if (upper !== null) { - return `x ≤ ${numberFormatter(upper)}`; - } - return ''; - }; - - const getMarks = ( - lower: number | null, - upper: number | null, - ): { [key: number]: string } => { - const newMarks: { [key: number]: string } = {}; - if (lower !== null) { - newMarks[lower] = numberFormatter(lower); - } - if (upper !== null) { - newMarks[upper] = numberFormatter(upper); - } - return newMarks; - }; + const getBounds = useCallback( + ( + value: [number, number], + ): { lower: number | null; upper: number | null } => { + const [lowerRaw, upperRaw] = value; + return { + lower: lowerRaw > min ? lowerRaw : null, + upper: upperRaw < max ? upperRaw : null, + }; + }, + [max, min], + ); - const handleAfterChange = (value: [number, number]): void => { - setValue(value); - const { lower, upper } = getBounds(value); - setMarks(getMarks(lower, upper)); + const handleAfterChange = useCallback( + (value: [number, number]): void => { + setValue(value); + const { lower, upper } = getBounds(value); + setMarks(getMarks(lower, upper)); - setDataMask({ - extraFormData: getRangeExtraFormData(col, lower, upper), - filterState: { - value: lower !== null || upper !== null ? value : null, - label: getLabel(lower, upper), - }, - }); - }; + setDataMask({ + extraFormData: getRangeExtraFormData(col, lower, upper), + filterState: { + value: lower !== null || upper !== null ? value : null, + label: getLabel(lower, upper), + }, + }); + }, + [col, getBounds, setDataMask], + ); - const handleChange = (value: [number, number]) => { + const handleChange = useCallback((value: [number, number]) => { setValue(value); - }; + }, []); useEffect(() => { // when switch filter type and queriesData still not updated we need ignore this case (in FilterBar) @@ -160,20 +167,25 @@ export default function RangeFilterPlugin(props: PluginFilterRangeProps) { handleAfterChange(filterState.value ?? [min, max]); }, [JSON.stringify(filterState.value), JSON.stringify(data)]); - const formItemData: FormItemProps = {}; - if (filterState.validateMessage) { - formItemData.extra = ( - - {filterState.validateMessage} - - ); - } + const formItemExtra = useMemo(() => { + if (filterState.validateMessage) { + return ( + + {filterState.validateMessage} + + ); + } + return undefined; + }, [filterState.validateMessage, filterState.validateStatus]); + + const minMax = useMemo(() => value ?? [min, max], [max, min, value]); + return ( {Number.isNaN(Number(min)) || Number.isNaN(Number(max)) ? (

{t('Chosen non-numeric column')}

) : ( - + numberFormatter(value)} + tipFormatter={tipFormatter} marks={marks} /> diff --git a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx index 6ae7cbeb225f..68475ec0f2b9 100644 --- a/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx +++ b/superset-frontend/src/filters/components/Select/SelectFilterPlugin.tsx @@ -34,7 +34,6 @@ import { Select } from 'src/components'; import debounce from 'lodash/debounce'; import { SLOW_DEBOUNCE } from 'src/constants'; import { useImmerReducer } from 'use-immer'; -import { FormItemProps } from 'antd/lib/form'; import { PluginFilterSelectProps, SelectValue } from './types'; import { StyledFormItem, FilterPluginStyle, StatusMessage } from '../common'; import { getDataRecordFormatter, getSelectExtraFormData } from '../../utils'; @@ -174,13 +173,16 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { [], ); - const searchWrapper = (val: string) => { - if (searchAllOptions) { - debouncedOwnStateFunc(val); - } - }; + const searchWrapper = useCallback( + (val: string) => { + if (searchAllOptions) { + debouncedOwnStateFunc(val); + } + }, + [debouncedOwnStateFunc, searchAllOptions], + ); - const clearSuggestionSearch = () => { + const clearSuggestionSearch = useCallback(() => { if (searchAllOptions) { dispatchDataMask({ type: 'ownState', @@ -190,21 +192,24 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { }, }); } - }; + }, [dispatchDataMask, initialColtypeMap, searchAllOptions]); - const handleBlur = () => { + const handleBlur = useCallback(() => { clearSuggestionSearch(); unsetFocusedFilter(); - }; + }, [clearSuggestionSearch, unsetFocusedFilter]); - const handleChange = (value?: SelectValue | number | string) => { - const values = ensureIsArray(value); - if (values.length === 0) { - updateDataMask(null); - } else { - updateDataMask(values); - } - }; + const handleChange = useCallback( + (value?: SelectValue | number | string) => { + const values = ensureIsArray(value); + if (values.length === 0) { + updateDataMask(null); + } else { + updateDataMask(values); + } + }, + [updateDataMask], + ); useEffect(() => { if (defaultToFirstItem && filterState.value === undefined) { @@ -245,14 +250,16 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) { ? t('No data') : tn('%s option', '%s options', data.length, data.length); - const formItemData: FormItemProps = {}; - if (filterState.validateMessage) { - formItemData.extra = ( - - {filterState.validateMessage} - - ); - } + const formItemExtra = useMemo(() => { + if (filterState.validateMessage) { + return ( + + {filterState.validateMessage} + + ); + } + return undefined; + }, [filterState.validateMessage, filterState.validateStatus]); const options = useMemo(() => { const options: { label: string; value: DataRecordValue }[] = []; @@ -270,7 +277,7 @@ export default function PluginFilterSelect(props: PluginFilterSelectProps) {