kgabryje commented on a change in pull request #16994:
URL: https://github.com/apache/superset/pull/16994#discussion_r723932988
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
dataMask: dataMaskSelected[filter.id],
}));
return buildCascadeFiltersTree(filtersWithValue);
- }, [filterValues, dataMaskSelected]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [JSON.stringify(filterValues), dataMaskSelected]);
Review comment:
Can we wrap `filterValues` in line 51 in useMemo instead? Like
```
const filterValues = useMemo(() => Object.values<Filter>(filters),
[filters]);
```
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
##########
@@ -105,10 +105,17 @@ const FilterValue: React.FC<FilterProps> = ({
time_range,
});
const filterOwnState = filter.dataMask?.ownState || {};
+ // TODO: We should try to improve our useEffect hooks to depend more on
+ // granular information instead of big objects that require deep
comparison.
+ const customizer = (
+ objValue: Partial<QueryFormData>,
+ othValue: Partial<QueryFormData>,
+ key: string,
+ ) => (key === 'url_params' ? true : undefined);
if (
!isRefreshing &&
- (!areObjectsEqual(formData, newFormData) ||
- !areObjectsEqual(ownState, filterOwnState) ||
+ (!isEqualWith(formData, newFormData, customizer) ||
Review comment:
Maybe we could enhance `areObjectsEqual` to accept customizer function?
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
dataMask: dataMaskSelected[filter.id],
}));
return buildCascadeFiltersTree(filtersWithValue);
- }, [filterValues, dataMaskSelected]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [JSON.stringify(filterValues), dataMaskSelected]);
Review comment:
Ah, now I see that `useFilters` hooks returns `Object.entries` which
creates a new object on each run... Yeah, that means that it probably needs
`JSON.stringify`, but in general I'm not sure if the pattern used in
`useFilters` is good
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
dataMask: dataMaskSelected[filter.id],
}));
return buildCascadeFiltersTree(filtersWithValue);
- }, [filterValues, dataMaskSelected]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [JSON.stringify(filterValues), dataMaskSelected]);
Review comment:
Maybe we could utilise a comparison function in `useSelector` (in
`useFilters` hook)? https://react-redux.js.org/api/hooks#useselector
##########
File path:
superset-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
##########
@@ -63,7 +63,8 @@ const FilterControls: FC<FilterControlsProps> = ({
dataMask: dataMaskSelected[filter.id],
}));
return buildCascadeFiltersTree(filtersWithValue);
- }, [filterValues, dataMaskSelected]);
+ // eslint-disable-next-line react-hooks/exhaustive-deps
+ }, [JSON.stringify(filterValues), dataMaskSelected]);
Review comment:
However, that might as well be a part of some refactor PR. I'm fine with
using `JSON.stringify` here as getting rid of those redundant queries has much
higher priority 👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
