fix: Unnecessary queries when changing filter values #16994
Conversation
Codecov Report
@@ Coverage Diff @@
## master #16994 +/- ##
==========================================
- Coverage 76.92% 76.92% -0.01%
==========================================
Files 1030 1030
Lines 55022 55025 +3
Branches 7465 7466 +1
==========================================
+ Hits 42328 42329 +1
- Misses 12440 12442 +2
Partials 254 254
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -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]); | |||
There was a problem hiding this comment.
Can we wrap filterValues in line 51 in useMemo instead? Like
const filterValues = useMemo(() => Object.values<Filter>(filters), [filters]);
There was a problem hiding this comment.
Can we wrap
filterValuesin line 51 in useMemo instead? Likeconst 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.
There was a problem hiding this 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
There was a problem hiding this comment.
Maybe we could utilise a comparison function in useSelector (in useFilters hook)? https://react-redux.js.org/api/hooks#useselector
There was a problem hiding this 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 👍
There was a problem hiding this comment.
Ah, now I see that
useFiltershooks returnsObject.entrieswhich creates a new object on each run... Yeah, that means that it probably needsJSON.stringify, but in general I'm not sure if the pattern used inuseFiltersis 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.
There was a problem hiding this comment.
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.
| if ( | ||
| !isRefreshing && | ||
| (!areObjectsEqual(formData, newFormData) || | ||
| !areObjectsEqual(ownState, filterOwnState) || | ||
| (!isEqualWith(formData, newFormData, customizer) || |
There was a problem hiding this comment.
Maybe we could enhance areObjectsEqual to accept customizer function?
There was a problem hiding this comment.
Maybe we could enhance
areObjectsEqualto accept customizer function?
Agreed, let's do that by adding customizer to opts in areObjectsEqual.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
|
🏷 v1.4 |
(cherry picked from commit c471a85)
mistercrunch
added
🍒 1.4.0
🍒 1.4.1
🍒 1.4.2
🏷️ bot
SUMMARY
The native filters were firing unnecessary queries when their values changed. This was happening because an
useEffecthook uses theformDataas a dependency and theurl_paramsproperty changes every time a value changes. To avoid that, I removed theurl_paramsfrom the comparison but we should definitely improve ouruseEffectdependencies to use more granular information instead of big objects that require deep comparison. Added aTODOin the code for future refactoring.I also changed the dependency in the calculation of the cascade filters tree to avoid unnecessary runs.
@rusackas @villebro @graceguo-supercat @junlincc
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2021-10-06.at.3.43.23.PM.mov
Screen.Recording.2021-10-06.at.3.29.56.PM.mov
TESTING INSTRUCTIONS
Check the videos for instructions.
ADDITIONAL INFORMATION