This repository has been archived by the owner on Feb 23, 2024. It is now read-only.
Fix Featured Product Search not working for large stores #5156
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexflorisca
added
type: bug
rubikuserbot
requested review from
a team and
ralucaStan
and removed request for
a team
|
Size Change: +4.76 kB (0%) Total Size: 1.12 MB
ℹ️ View Unchanged
|
senadir
reviewed
Nov 19, 2021
| @@ -48,7 +48,7 @@ const withSearchedProducts = ( | |||
| setIsLoading( false ); | |||
| } ) | |||
| .catch( setErrorState ); | |||
| }, [ selected ] ); | |||
| }, [] ); | |||
There was a problem hiding this comment.
This is an anti pattern that we fought a bit to get rid off and refactor away from, because it causes hard to debug bugs.
If you feel like selected is not going to change much here, I'd suggest you have it a ref then?
const selectedRef = useRef( selected );
selectedRef.current;This should achieve the same goal, and would not cause rerenders.
There was a problem hiding this comment.
Thanks for pointing that out @senadir, I'd be curious to know more about the types of bugs this causes! I've changed the implementation to use a ref as you suggested.
|
Thanks, that’s interesting, thanks for sharing. I’m going to read up more about it. I was under the impression that if the argument list is empty, it will actually avoid re-renders (only once) so it’s a good way to say “I only want this to run once”.
…On 23 Nov 2021, 13:50 +0000, Seghir Nadir ***@***.***>, wrote:
@senadir commented on this pull request.
In assets/js/hocs/with-searched-products.tsx:
> @@ -48,7 +48,7 @@ const withSearchedProducts = (
setIsLoading( false );
} )
.catch( setErrorState );
- }, [ selected ] );
+ }, [] );
Hey! This causes out of sync bugs, things running when not needed, and generally hard to find bugs, examples:
#3314
#3285
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
opr
reviewed
Nov 29, 2021
There was a problem hiding this comment.
Is it expected to get two (debounced) requests every time you type into the search box? I get the following two:
/wp-json/wc/store/products?per_page=5&catalog_visibility=any&search=beanie&orderby=title&order=asc&_locale=user/wp-json/wc/store/products?catalog_visibility=any&include%5B0%5D=0&per_page=0&_locale=user
It looks like the |
Yes, that's still the case, however, we want to avoid diverging from that eslint rule, not adding things to the deps rule to achieve that indicates a code smell and an issue to fix higher up, or with an alternate solution. |
opr
approved these changes
Dec 2, 2021
There was a problem hiding this comment.
it looks like the getProductsRequest method ads another request here - it makes another request to get the currently selected item, because searching again on a large store will change the list of results based on search and may remove the currently selected item from the dom. So it looks like it's meant to be there :)
OK thanks! If this second request is necessary then I'm happy to approve. 🚢 👍🏼
jonny-bull
pushed a commit
to jonny-bull/woocommerce-gutenberg-products-block
that referenced
this pull request
Dec 14, 2021
…#5156) * Remove the dependency from the getProducts useEffect * using ref instead of empty dependency array for useEffect
jonny-bull
pushed a commit
to jonny-bull/woocommerce-gutenberg-products-block
that referenced
this pull request
Dec 16, 2021
…#5156) * Remove the dependency from the getProducts useEffect * using ref instead of empty dependency array for useEffect
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
On the editor, after inserting a featured product block, the search does not return any results for large stores (with more than 100 products currently). This is due to the behaviour of the
WithSearchedProductshigher order component. This generates a request to fetch products, both on a user searching, and every time the user changes a selection.What's happening is this useEffect is ran on (almost) every render, because
selectedis an array that is checked by reference and it will have a different array instance each time the component is invoked, even if the value of the array hasn't changed.The solution proposed here simply removes
selectedfrom the dependency array so thatgetProductswill only run once on the first render. From what I can see, there is no need togetProductsafter each selection, and there is a separate call togetProductsin theonSearchcallback. This looks to me like the intended behaviour but please point out if I'm missing something.I've also explored memoizing the
selectedarray withuseMemo()but this still doesn't work, presumably because it's a different array each time.Yet another solution would be to use
JSON.stringify(selected)to check for the change in value of theselectedarray as described here. We could do this if thisuseEffectneeds to run on every product selection.Also eslint is throwing a warning about
React Hook useEffect has a missing dependency: 'selected'. Either include it or remove the dependency array. But for this situation I think this is the right setting so I would disable the warning for this line but wanted to check first. Open to suggestions on this.Fixes #4983
Screenshots
Before:
After:
Testing
Manual Testing
How to test the changes in this Pull Request:
Performance Impact
Changelog