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
Implement Cache Logic for Shopper APIs (Contexts/Customers/Login/Orders) #1073
Implement Cache Logic for Shopper APIs (Contexts/Customers/Login/Orders) #1073
Changes from 21 commits
00c402a
97b0190
20dfc21
5dc64b5
b3a55a8
c85a14a
3d9002e
47df510
97359e3
d04745b
f20ef61
522196f
9a950fc
e7bd07c
a4b9191
c685f96
4b467a2
1e83f14
5835c4d
d378615
d15b5de
42c1fe1
d146e2f
2f6f01a
0d2b555
e4ec495
2229c09
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.
Do we want to add a TODO as a reminder that we need to invalidate all the cache in case data stored in cache is affected by the changes to the 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.
I added a TODO in a comment with a link to the ticket for the work. I chose not to use the TODO method since this work is follow up work and not a developer reminder before going GA
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 might see these todo's left around, they are simply copied and pasted but for the most part are still valid for future cache work.
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.
Aside: I think we should not do this selective updates in the future.
I've tried doing it in one of my PRs last time. But I reverted the change, as the logic became complex and error-prone in my experience.
Take a read of this article from one of the core maintainers of react-query. He suggested to prefer invalidation over direct updates (which is similar to "selective updates" in our terminology), because invalidation is safer and simpler approach.
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.
@vmarta I had the exact same thought as your suggestion, I previously said that
But later Alex found out an issue that made me think twice about the statement. The reason being that in SCAPI land, the models are used interchangeably across different API endpoints. For example, you get shopper's baskets from ShopperCustomer API as oppose to ShopperBasket API, but you mutate the baskets using ShopperBasket API. This creates dependencies between queries. For example, it's common that we have the following hooks on a page
When the mutation happens, if we don't update, but only invalidate, we run into a situation that we don't have access to the invalidation fetch call, so loading state is broken for all mutations.
I'm not sure if there is another way to solve this, selective updates seems like the obvious solution...