Fix React hook dependency warnings in filter an All Products blocks #3285
Conversation
Aljullu
added
status: needs review
type: refactor
|
Size Change: +577 B (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
|
Note that I'm aware of this PR and will get to it in the next couple of days unless it's blocking any other work or there's a risk it will get outdated quickly, in which case I can review it today? |
|
Sounds good @senadir. I don't think there is any hurry as long as it's dealt with before the end of cooldown. 🙂 |
Aljullu
force-pushed
the
refactor/fix-hook-dependency-warnings-filters-all-products
branch
from
08e3a96 to
0d4fd7a
Compare
There was a problem hiding this comment.
I'm actually a bit scared of merging this 😅
It's a lot of changes that are not obvious to me, I can see possible hidden regressions in the future.
I followed the testing steps and nothing broke, so we're comfortable with that.
I can't think of any other testing scenario.
So, I'm going to approve this and would be happy to merge it.
However, if you have any free time now or later, I would really appreciate some comments here and there to justify the changes, it would be helpful for future developers if we track a bug down to this PR.
Usually, I would suggest doing that in a follow-up issue, but the more we delay this, the greater chances that you forget why you did those changes.
There was a problem hiding this comment.
Thanks for the review @senadir. 💯 that it's very tricky to understand these changes, in fact it was a really complex PR to write. Also, they are blocks that we didn't touch for some time, so it required some 'context switching'.
I added inline comments to all changes included in this PR, I hope they will make it easier to understand the rationale behind the changes.
| @@ -142,6 +142,7 @@ const CheckboxList = ( { | |||
| ); | |||
| }, [ | |||
| options, | |||
| onChange, | |||
There was a problem hiding this comment.
This one was easy: missing dependency in a useMemo. onChange is not expected to change, but in order to fix the linting warning, it was needed.
| @@ -186,7 +185,14 @@ const PriceSlider = ( { | |||
| parseInt( values[ 1 ], 10 ), | |||
| ] ); | |||
| }, | |||
| [ minPrice, maxPrice, minConstraint, maxConstraint, stepValue ] | |||
| [ | |||
| onChange, | |||
There was a problem hiding this comment.
Similar as before, but in this case it's a useCallback().
| maxPriceInput, | ||
| currency, | ||
| ] | ||
| [ onChange, stepValue, minPriceInput, maxPriceInput ] |
There was a problem hiding this comment.
onChange was a missing dependency, while minConstraint, maxConstraint and currency were listed as a dependency, but they are not used inside the callback.
| @@ -96,6 +96,11 @@ const announceLoadingCompletion = ( totalProducts ) => { | |||
| } | |||
| }; | |||
|
|
|||
| const areQueryTotalsDifferent = ( | |||
There was a problem hiding this comment.
I extracted that function outside of the functional component so it's not re-created on every render. This prevents usePrevious() repeating the validation unnecessarily on every render if the value didn't change. See the changes in use-previous.js here:
| Number.isFinite( val ) | ||
| const previousConstraint = usePrevious( | ||
| currentConstraint, | ||
| Number.isFinite |
There was a problem hiding this comment.
Avoids creating a new function on every render, which would cause usePrevious() running the validation function unnecessarily.
| @@ -258,45 +258,42 @@ const Edit = ( { attributes, setAttributes, debouncedSpeak } ) => { | |||
| </Placeholder> | |||
| ); | |||
|
|
|||
| const onDone = useCallback( () => { | |||
| const onDone = () => { | |||
There was a problem hiding this comment.
I don't think it made much sense wrapping this function in a useCallback() because it's not used inside any other hook.
| if ( ! selected || ! selected.length ) { | ||
| return; | ||
| } | ||
| const onChange = ( selected ) => { |
| @@ -26,10 +27,12 @@ import usePriceConstraints from './use-price-constraints.js'; | |||
| */ | |||
| const PriceFilterBlock = ( { attributes, isEditor = false } ) => { | |||
| const [ minPriceQuery, setMinPriceQuery ] = useQueryStateByKey( | |||
There was a problem hiding this comment.
useQueryStateByKey() sets the default value to an empty object, I added the null parameter so the default value is null, this avoids unnecessary re-calculations when using minPriceQuery and maxPriceQuery in hook dependencies.
| @@ -73,29 +81,61 @@ const PriceFilterBlock = ( { attributes, isEditor = false } ) => { | |||
| setMaxPrice( prices[ 1 ] ); | |||
| } | |||
| }, | |||
| [ minConstraint, maxConstraint, minPrice, maxPrice ] | |||
| [ minPrice, maxPrice, setMinPrice, setMaxPrice ] | |||
There was a problem hiding this comment.
Replacing wrong deps in this callback, this change should be safe.
| previousMaxConstraint, | ||
| previousMinPriceQuery, | ||
| previousMaxPriceQuery, | ||
| ] ); |
There was a problem hiding this comment.
This is probably the most complex change in this PR, this effect was relying on missing deps to be run only when the min/max query price or the min/max constraint changed. However, in order to fix the linting warnings we needed to add several missing dependencies. That mean the effect run more often than before, that's why I needed to add extra checks to ensure the query price or the constraints had changed.
Aljullu
deleted the
refactor/fix-hook-dependency-warnings-filters-all-products
branch
|
Excellent explanation @Aljullu, Thank you for taking the time to write it. |
Part of #3204.
This PR handles the React hook dependency warnings produced by ESLint in the filter and All Products blocks. Some fixes were trivial while some others needed bigger refactors. Adding new dependencies to effects/memos/callbacks might have caused them to do unnecessary recalculations, when that was evident, I tried to prevent it either wrapping some dependencies with
useShallowEqual()or refactoring it completely. However, I might have missed some and for sure there are many more optimizations to do.How to test the changes in this Pull Request:
The scope of changes in this PR is limited to the filter and All Products blocks, so those are the ones that need to be tested.
In the process I found a couple of bugs (Price slider goes back to 0 after moving the min handle #3282 and Price slider max constraint doesn't match price filter input value woocommerce#42675), but I could reproduce them in
trunktoo, so I filled issues in GH.