From bf001931c8c7e58a211e411fa74ca4991c6aa2a8 Mon Sep 17 00:00:00 2001 From: Stephen Liu <750188453@qq.com> Date: Sun, 30 Oct 2022 23:55:48 +0800 Subject: [PATCH] perf(native-filters): improve native filter modal form performance (#21821) --- .../FilterConfigPane.test.tsx | 8 - .../FilterConfigurePane.tsx | 56 ++---- .../FiltersConfigForm/FiltersConfigForm.tsx | 9 +- .../FiltersConfigForm/utils.ts | 8 +- .../FiltersConfigModal/FiltersConfigModal.tsx | 173 ++++++++++++------ 5 files changed, 148 insertions(+), 106 deletions(-) diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx index 3742d536326f..223d5a35d00d 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigPane.test.tsx @@ -26,7 +26,6 @@ const scrollMock = jest.fn(); Element.prototype.scroll = scrollMock; const defaultProps = { - children: jest.fn(), getFilterTitle: (id: string) => id, onChange: jest.fn(), onAdd: jest.fn(), @@ -63,13 +62,6 @@ beforeEach(() => { scrollMock.mockClear(); }); -test('renders form', async () => { - await act(async () => { - defaultRender(); - }); - expect(defaultProps.children).toHaveBeenCalledTimes(3); -}); - test('drag and drop', async () => { defaultRender(); // Drag the state and country filter above the product filter diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx index dba7e6bb3025..d02a6ba2c316 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FilterConfigurePane.tsx @@ -22,7 +22,7 @@ import FilterTitlePane from './FilterTitlePane'; import { FilterRemoval } from './types'; interface Props { - children: (filterId: string) => React.ReactNode; + children?: React.ReactNode; getFilterTitle: (filterId: string) => string; onChange: (activeKey: string) => void; onAdd: (type: NativeFilterType) => void; @@ -62,40 +62,24 @@ const FilterConfigurePane: React.FC = ({ currentFilterId, filters, removedFilters, -}) => { - const active = filters.filter(id => id === currentFilterId)[0]; - return ( - - - onAdd(type)} - onRearrange={onRearrange} - onRemove={(id: string) => onRemove(id)} - restoreFilter={restoreFilter} - /> - - - {filters.map(id => ( -
- {children(id)} -
- ))} - - - ); -}; +}) => ( + + + onAdd(type)} + onRearrange={onRearrange} + onRemove={(id: string) => onRemove(id)} + restoreFilter={restoreFilter} + /> + + {children} + +); export default FilterConfigurePane; diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx index 43901ddec291..24f93ae38bd0 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/FiltersConfigForm.tsx @@ -332,6 +332,7 @@ const FiltersConfigForm = ( setErroredFilters, validateDependencies, getDependencySuggestion, + isActive, }: FiltersConfigFormProps, ref: React.RefObject, ) => { @@ -346,7 +347,7 @@ const FiltersConfigForm = ( string, any > | null>(null); - const forceUpdate = useForceUpdate(); + const forceUpdate = useForceUpdate(isActive); const [datasetDetails, setDatasetDetails] = useState>(); const defaultFormFilter = useMemo(() => ({}), []); const filters = form.getFieldValue('filters'); @@ -1240,6 +1241,8 @@ const FiltersConfigForm = ( ); }; -export default forwardRef( - FiltersConfigForm, +export default React.memo( + forwardRef( + FiltersConfigForm, + ), ); diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts index 2a0b7fcad857..77e27a42110c 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigForm/utils.ts @@ -38,9 +38,13 @@ export const FILTER_SUPPORTED_TYPES = { filter_range: [GenericDataType.NUMERIC], }; -export const useForceUpdate = () => { +export const useForceUpdate = (isActive = true) => { const [, updateState] = React.useState({}); - return React.useCallback(() => updateState({}), []); + return React.useCallback(() => { + if (isActive) { + updateState({}); + } + }, [isActive]); }; export const setNativeFilterFieldValues = ( diff --git a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx index fd4f7d550fe0..63bca5b3d166 100644 --- a/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx +++ b/superset-frontend/src/dashboard/components/nativeFilters/FiltersConfigModal/FiltersConfigModal.tsx @@ -153,12 +153,15 @@ export function FiltersConfigModal({ const unsavedFiltersIds = newFilterIds.filter(id => !removedFilters[id]); // brings back a filter that was previously removed ("Undo") - const restoreFilter = (id: string) => { - const removal = removedFilters[id]; - // gotta clear the removal timeout to prevent the filter from getting deleted - if (removal?.isPending) clearTimeout(removal.timerId); - setRemovedFilters(current => ({ ...current, [id]: null })); - }; + const restoreFilter = useCallback( + (id: string) => { + const removal = removedFilters[id]; + // gotta clear the removal timeout to prevent the filter from getting deleted + if (removal?.isPending) clearTimeout(removal.timerId); + setRemovedFilters(current => ({ ...current, [id]: null })); + }, + [removedFilters], + ); const getInitialFilterOrder = () => Object.keys(filterConfigMap); // State for tracking the re-ordering of filters @@ -166,6 +169,11 @@ export function FiltersConfigModal({ getInitialFilterOrder(), ); + // State for rendered filter to improve performance + const [renderedFilters, setRenderedFilters] = useState([ + initialCurrentFilterId, + ]); + const getActiveFilterPanelKey = (filterId: string) => [ `${filterId}-${FilterPanels.configuration.key}`, `${filterId}-${FilterPanels.settings.key}`, @@ -175,7 +183,7 @@ export function FiltersConfigModal({ string | string[] >(getActiveFilterPanelKey(initialCurrentFilterId)); - const onTabChange = (filterId: string) => { + const handleTabChange = (filterId: string) => { setCurrentFilterId(filterId); setActiveFilterPanelKey(getActiveFilterPanelKey(filterId)); }; @@ -229,6 +237,7 @@ export function FiltersConfigModal({ if (!isSaving) { setOrderedFilters(getInitialFilterOrder()); } + setRenderedFilters([initialCurrentFilterId]); form.resetFields(['filters']); form.setFieldsValue({ changed: false }); }; @@ -379,7 +388,7 @@ export function FiltersConfigModal({ handleConfirmCancel(); } }; - const onRearrange = (dragIndex: number, targetIndex: number) => { + const handleRearrange = (dragIndex: number, targetIndex: number) => { const newOrderedFilter = [...orderedFilters]; const removed = newOrderedFilter.splice(dragIndex, 1)[0]; newOrderedFilter.splice(targetIndex, 0, removed); @@ -405,7 +414,7 @@ export function FiltersConfigModal({ return dependencyMap; }, [filterConfigMap, form]); - const validateDependencies = () => { + const validateDependencies = useCallback(() => { const dependencyMap = buildDependencyMap(); filterIds .filter(id => !removedFilters[id]) @@ -418,26 +427,35 @@ export function FiltersConfigModal({ form.setFields([field]); }); handleErroredFilters(); - }; + }, [ + buildDependencyMap, + filterIds, + form, + handleErroredFilters, + removedFilters, + ]); - const getDependencySuggestion = (filterId: string) => { - const dependencyMap = buildDependencyMap(); - const possibleDependencies = orderedFilters.filter( - key => key !== filterId && canBeUsedAsDependency(key), - ); - const found = possibleDependencies.find(filter => { - const dependencies = dependencyMap.get(filterId) || []; - dependencies.push(filter); - if (hasCircularDependency(dependencyMap, filterId)) { - dependencies.pop(); - return false; - } - return true; - }); - return found || possibleDependencies[0]; - }; + const getDependencySuggestion = useCallback( + (filterId: string) => { + const dependencyMap = buildDependencyMap(); + const possibleDependencies = orderedFilters.filter( + key => key !== filterId && canBeUsedAsDependency(key), + ); + const found = possibleDependencies.find(filter => { + const dependencies = dependencyMap.get(filterId) || []; + dependencies.push(filter); + if (hasCircularDependency(dependencyMap, filterId)) { + dependencies.pop(); + return false; + } + return true; + }); + return found || possibleDependencies[0]; + }, + [buildDependencyMap, canBeUsedAsDependency, orderedFilters], + ); - const onValuesChange = useMemo( + const handleValuesChange = useMemo( () => debounce((changes: any, values: NativeFiltersForm) => { const didChangeFilterName = @@ -466,32 +484,73 @@ export function FiltersConfigModal({ ); }, [removedFilters]); - const getForm = (id: string) => { - const isDivider = id.startsWith(NATIVE_FILTER_DIVIDER_PREFIX); - return isDivider ? ( - - ) : ( - setActiveFilterPanelKey(key)} - isActive={currentFilterId === id} - setErroredFilters={setErroredFilters} - validateDependencies={validateDependencies} - getDependencySuggestion={getDependencySuggestion} - /> - ); - }; + useEffect(() => { + if (!renderedFilters.includes(currentFilterId)) { + setRenderedFilters([...renderedFilters, currentFilterId]); + } + }, [currentFilterId]); + + const handleActiveFilterPanelChange = useCallback( + key => setActiveFilterPanelKey(key), + [setActiveFilterPanelKey], + ); + + const formList = useMemo( + () => + orderedFilters.map(id => { + if (!renderedFilters.includes(id)) return null; + const isDivider = id.startsWith(NATIVE_FILTER_DIVIDER_PREFIX); + const isActive = currentFilterId === id; + return ( +
+ {isDivider ? ( + + ) : ( + + )} +
+ ); + }), + [ + renderedFilters, + orderedFilters, + currentFilterId, + filterConfigMap, + form, + removedFilters, + restoreFilter, + getAvailableFilters, + activeFilterPanelKey, + validateDependencies, + getDependencySuggestion, + handleActiveFilterPanelChange, + ], + ); return ( - {(id: string) => getForm(id)} + {formList}