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.
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.
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
Account's wishlist: integrate hooks #1042
Account's wishlist: integrate hooks #1042
Changes from 24 commits
11b6261
c862ffe
3923dc8
0b444d5
d72419c
a82ae6d
fbf6cba
428c3a3
6d8d9bb
a35170d
4dd1661
7fe8af0
dff593a
e51439c
5524928
da6129e
7900f52
907401a
c6ad3fc
3de2b35
49eb095
fa90831
20d0c2e
1f307c0
20c2186
a7fad37
1bd4a5c
f5159e2
6bd9593
3c47cf8
487c775
438a45c
df6e823
3792643
67c339f
1643906
60aecdf
b5c1db5
9fd05e0
c73b2e5
7c6e45e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget to remove the query from the invalidate list; invalidate takes precedence over update. (Technically, it will update then invalidate, which negates the point of updating.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after the list item is updated, the product lists become somewhat stale. For example, the
lastModified
time is no longer true.So I prefer to also invalidate the product lists. Yes, most of the data can be redundant, but the refetch is going to be done in the background. In some way, I think of it as similar to stale-while-revalidate strategy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we update and invalidate at the same time, does react query ever show the updated data? I thought that it just keeps the old data until the refetch completes. (I could very easily be wrong.)
Also, I just noticed that we update/invalidate the lists endpoint, but we just invalidate the list endpoint. We should follow the same pattern for both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You brought up good points there. To make things simpler, I've reverted the cache-update logic.
This article mentions that it's safer and simpler to just invalidate, rather than update. And I can see that here in our case.
So what now? There's actually another way to achieve what I wanted: via local state update in the quantity picker component. Take a look at this commit for details: 487c775
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On
develop
when the shopper clicks the "Add to Wishlist" button on one of the items of a product set, the individual item is added to the wishlist, whereas now the entire set is added to the wishlist instead of the individual product.Example product set:
https://pwa-kit.mobify-storefront.com/global/en-GB/product/fall-lookM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm strange, that might be a regression after resolving merge conflicts.. let me investigate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I've fixed this issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this state? can we replace this by the
isLoading
state from the mutation? (i might be missing some context)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question. That state is still necessary in 1 case (removing an item), but for another case (updating quantity), I've updated it so it used the mutation's loading state instead.
(The one case where it's still necessary is because the mutation lives in a different file/component)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will send a request if
productIds
is an empty array. Is that desired behavior?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I'll update it to check for an array and that it's not empty.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see https://github.com/SalesforceCommerceCloud/pwa-kit/pull/1042/files#r1135965135
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in
develop
removes the item from the wishlist when the shopper decrements the product quantity < 1, when taking either of these two actions in the QuantityPicker:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, the smart deletion logic originally comes from:
pwa-kit/packages/template-retail-react-app/app/commerce-api/hooks/useCustomerProductLists.js
Lines 204 to 210 in 487c775
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI I've implemented that.