perf(native-filters): Decrease number of unnecessary rerenders in nat…

…ive filters (apache#17115) * perf(native-filters): decrease number of redundant rerenders * More perf improvements * lint fix
opus-42 · Nov 14, 2021 · 7c9a748 · 7c9a748
1 parent 0588a21
commit 7c9a748
Show file tree

Hide file tree

Showing 12 changed files with 321 additions and 232 deletions.
diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx b/superset-frontend/src/dashboard/components/DashboardBuilder/DashboardContainer.tsx
@@ -45,9 +45,9 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
   const dashboardLayout = useSelector<RootState, DashboardLayout>(
     state => state.dashboardLayout.present,
   );
-  const nativeFilters =
-    useSelector<RootState, Filters>(state => state.nativeFilters?.filters) ??
-    {};
+  const nativeFilters = useSelector<RootState, Filters>(
+    state => state.nativeFilters?.filters,
+  );
   const directPathToChild = useSelector<RootState, string[]>(
     state => state.dashboardState.directPathToChild,
   );
@@ -68,7 +68,7 @@ const DashboardContainer: FC<DashboardContainerProps> = ({ topLevelTabs }) => {
   }, [getLeafComponentIdFromPath(directPathToChild)]);
 
   // recalculate charts and tabs in scopes of native filters only when a scope or dashboard layout changes
-  const filterScopes = Object.values(nativeFilters).map(filter => ({
+  const filterScopes = Object.values(nativeFilters ?? {}).map(filter => ({
     id: filter.id,
     scope: filter.scope,
   }));

diff --git a/superset-frontend/src/dashboard/components/DashboardBuilder/state.ts b/superset-frontend/src/dashboard/components/DashboardBuilder/state.ts
@@ -18,7 +18,7 @@
  */
 import { useSelector } from 'react-redux';
 import { FeatureFlag, isFeatureEnabled } from 'src/featureFlags';
-import { useEffect, useState } from 'react';
+import { useCallback, useEffect, useState } from 'react';
 import { URL_PARAMS } from 'src/constants';
 import { getUrlParam } from 'src/utils/urlUtils';
 import { RootState } from 'src/dashboard/types';
@@ -64,9 +64,12 @@ export const useNativeFilters = () => {
       )
     );
 
-  const toggleDashboardFiltersOpen = (visible?: boolean) => {
-    setDashboardFiltersOpen(visible ?? !dashboardFiltersOpen);
-  };
+  const toggleDashboardFiltersOpen = useCallback(
+    (visible?: boolean) => {
+      setDashboardFiltersOpen(visible ?? !dashboardFiltersOpen);
+    },
+    [dashboardFiltersOpen],
+  );
 
   useEffect(() => {
     if (

diff --git a/.../src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx b/.../src/dashboard/components/nativeFilters/FilterBar/CascadeFilters/CascadePopover/index.tsx
@@ -225,5 +225,4 @@ const CascadePopover: React.FC<CascadePopoverProps> = ({
     </Popover>
   );
 };
-
-export default CascadePopover;
+export default React.memo(CascadePopover);
diff --git a/...rontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControl.tsx b/...rontend/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<FilterProps> = ({
     filter.dataMask?.filterState,
   );
 
+  const label = useMemo(
+    () => (
+      <StyledFilterControlTitleBox>
+        <StyledFilterControlTitle data-test="filter-control-name">
+          {name}
+        </StyledFilterControlTitle>
+        <StyledIcon data-test="filter-icon">{icon}</StyledIcon>
+      </StyledFilterControlTitleBox>
+    ),
+    [icon, name],
+  );
+
   return (
     <StyledFilterControlContainer layout="vertical">
       <FormItem
-        label={
-          <StyledFilterControlTitleBox>
-            <StyledFilterControlTitle data-test="filter-control-name">
-              {name}
-            </StyledFilterControlTitle>
-            <StyledIcon data-test="filter-icon">{icon}</StyledIcon>
-          </StyledFilterControlTitleBox>
-        }
+        label={label}
         required={filter?.controlValues?.enableEmptyFilter}
         validateStatus={isMissingRequiredValue ? 'error' : undefined}
       >
@@ -92,5 +97,4 @@ const FilterControl: React.FC<FilterProps> = ({
     </StyledFilterControlContainer>
   );
 };
-
-export default FilterControl;
+export default React.memo(FilterControl);
diff --git a/...ontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx b/...ontend/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<FilterControlsProps> = ({
 }) => {
   const [visiblePopoverId, setVisiblePopoverId] = useState<string | null>(null);
   const filters = useFilters();
-  const filterValues = Object.values<Filter>(filters);
-  const portalNodes = React.useMemo(() => {
+  const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
+  const portalNodes = useMemo(() => {
     const nodes = new Array(filterValues.length);
     for (let i = 0; i < filterValues.length; i += 1) {
       nodes[i] = createHtmlPortalNode();
@@ -70,8 +70,7 @@ const FilterControls: FC<FilterControlsProps> = ({
       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 [filtersInScope, filtersOutOfScope] = useSelectFiltersInScope(
@@ -80,26 +79,36 @@ const FilterControls: FC<FilterControlsProps> = ({
   const dashboardHasTabs = useDashboardHasTabs();
   const showCollapsePanel = dashboardHasTabs && cascadeFilters.length > 0;
 
+  const cascadePopoverFactory = useCallback(
+    index => (
+      <CascadePopover
+        data-test="cascade-filters-control"
+        key={cascadeFilters[index].id}
+        dataMaskSelected={dataMaskSelected}
+        visible={visiblePopoverId === cascadeFilters[index].id}
+        onVisibleChange={visible =>
+          setVisiblePopoverId(visible ? cascadeFilters[index].id : null)
+        }
+        filter={cascadeFilters[index]}
+        onFilterSelectionChange={onFilterSelectionChange}
+        directPathToChild={directPathToChild}
+        inView={false}
+      />
+    ),
+    [
+      cascadeFilters,
+      JSON.stringify(dataMaskSelected),
+      directPathToChild,
+      onFilterSelectionChange,
+      visiblePopoverId,
+    ],
+  );
   return (
     <Wrapper>
       {portalNodes
         .filter((node, index) => cascadeFilterIds.has(filterValues[index].id))
         .map((node, index) => (
-          <InPortal node={node}>
-            <CascadePopover
-              data-test="cascade-filters-control"
-              key={cascadeFilters[index].id}
-              dataMaskSelected={dataMaskSelected}
-              visible={visiblePopoverId === cascadeFilters[index].id}
-              onVisibleChange={visible =>
-                setVisiblePopoverId(visible ? cascadeFilters[index].id : null)
-              }
-              filter={cascadeFilters[index]}
-              onFilterSelectionChange={onFilterSelectionChange}
-              directPathToChild={directPathToChild}
-              inView={false}
-            />
-          </InPortal>
+          <InPortal node={node}>{cascadePopoverFactory(index)}</InPortal>
         ))}
       {filtersInScope.map(filter => {
         const index = filterValues.findIndex(f => f.id === filter.id);
@@ -150,4 +159,4 @@ const FilterControls: FC<FilterControlsProps> = ({
   );
 };
 
-export default FilterControls;
+export default React.memo(FilterControls);
diff --git a/...-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx b/...-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<FilterProps> = ({
   dataMaskSelected,
   filter,
@@ -193,11 +202,36 @@ const FilterValue: React.FC<FilterProps> = ({
     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 hooks = useMemo(
+    () => ({ setDataMask, setFocusedFilter, unsetFocusedFilter }),
+    [setDataMask, setFocusedFilter, unsetFocusedFilter],
+  );
+
+  const isMissingRequiredValue = checkIsMissingRequiredValue(
+    filter,
+    filter.dataMask?.filterState,
+  );
 
-  const setFocusedFilter = () => dispatchFocusAction(dispatch, id);
-  const unsetFocusedFilter = () => dispatchFocusAction(dispatch);
+  const filterState = useMemo(
+    () => ({
+      ...filter.dataMask?.filterState,
+      validateStatus: isMissingRequiredValue && 'error',
+    }),
+    [filter.dataMask?.filterState, isMissingRequiredValue],
+  );
 
   if (error) {
     return (
@@ -208,14 +242,6 @@ const FilterValue: React.FC<FilterProps> = ({
       />
     );
   }
-  const isMissingRequiredValue = checkIsMissingRequiredValue(
-    filter,
-    filter.dataMask?.filterState,
-  );
-  const filterState = {
-    ...filter.dataMask?.filterState,
-    validateStatus: isMissingRequiredValue && 'error',
-  };
 
   return (
     <StyledDiv data-test="form-item-value">
@@ -227,18 +253,17 @@ const FilterValue: React.FC<FilterProps> = ({
           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}
         />
       )}
     </StyledDiv>
   );
 };
-
 export default FilterValue;
diff --git 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]);
 }