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.

Sign up for GitHub

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

fix: Unnecessary queries when changing filter values #16994

Merged
merged 1 commit into from
Oct 7, 2021
Merged

fix: Unnecessary queries when changing filter values #16994

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion ...ontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterControls.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we wrap filterValues in line 51 in useMemo instead? Like

const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we wrap filterValues in line 51 in useMemo instead? Like

const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);

That's a good idea - however, won't we need to use stringify filter in the deps array? Seems like the object reference may change without the contents actually changing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could utilise a comparison function in useSelector (in useFilters hook)? https://react-redux.js.org/api/hooks#useselector

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

I agree, it's definitely an antipattern. However, until we have a guideline for avoiding them I'd suggest going with JSON.stringify for now, leaving a // TODO: here and then cleaning these stringifys up in follow-up refactor PRs once we've settled on a good pattern for being able to use strict equality in the deps arrays.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Let's use JSON.stringify for now and plan to deal with all occurrences of it in a specific refactor effort. Meanwhile, we should avoid using objects that require deep comparison as useEffect dependencies.

const cascadeFilterIds = new Set(cascadeFilters.map(item => item.id));

const [filtersInScope, filtersOutOfScope] = useSelectFiltersInScope(
Expand Down
13 changes: 10 additions & 3 deletions ...-frontend/src/dashboard/components/nativeFilters/FilterBar/FilterControls/FilterValue.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
getChartMetadataRegistry,
} from '@superset-ui/core';
import { useDispatch, useSelector } from 'react-redux';
import { areObjectsEqual } from 'src/reduxUtils';
import { isEqual, isEqualWith } from 'lodash';
import { getChartDataRequest } from 'src/chart/chartAction';
import Loading from 'src/components/Loading';
import BasicErrorAlert from 'src/components/ErrorMessage/BasicErrorAlert';
Expand Down Expand Up @@ -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) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could enhance areObjectsEqual to accept customizer function?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could enhance areObjectsEqual to accept customizer function?

Agreed, let's do that by adding customizer to opts in areObjectsEqual.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I agree. This is the implementation of areObjectsEqual:

export function areObjectsEqual(
  obj1: any,
  obj2: any,
  opts = { ignoreUndefined: false },
) {
  let comp1 = obj1;
  let comp2 = obj2;
  if (opts.ignoreUndefined) {
    comp1 = omitBy(obj1, isUndefined);
    comp2 = omitBy(obj2, isUndefined);
  }
  return isEqual(comp1, comp2);
}

The first thing is that is very inefficient because it creates new objects (omitBy) only for the purpose of removing the properties that are undefined. The second thing is that this seems to be a customizer implementation and not a new version of isEqual. I think the best approach would be to have pre-defined customizers to use with Lodash isEqualWith. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of areObjectsEqual IMO is to serve as a minimum-effort wrapper around isEqual that can be centrally updated if the lodash API were to change (I believe I may have implemented that ignoreUndefined some time ago based on some docs I found). I'm all for making this more efficient - if this can be implemented in a simpler fashion let's do it 👍

!isEqual(ownState, filterOwnState) ||
isDashboardRefreshing)
) {
setFormData(newFormData);
Expand Down