-
Notifications
You must be signed in to change notification settings - Fork 142
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
Changes from 23 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
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,11 +19,6 @@ import {and, pathStartsWith} from '../utils' | |
type Client = ApiClients['shopperCustomers'] | ||
|
||
const noop = () => ({}) | ||
/** Logs a warning to console (on startup) and returns nothing (method is unimplemented). */ | ||
const TODO = (method: keyof Client) => { | ||
console.warn(`Cache logic for '${method}' is not yet implemented.`) | ||
return undefined | ||
} | ||
|
||
/** Invalidates the customer endpoint, but not derivative endpoints. */ | ||
const invalidateCustomer = (parameters: Tail<QueryKeys['getCustomer']>): CacheUpdate => ({ | ||
|
@@ -84,7 +79,13 @@ export const cacheUpdateMatrix: CacheUpdateMatrix<Client> = { | |
remove: [{queryKey: getCustomerPaymentInstrument.queryKey(parameters)}] | ||
} | ||
}, | ||
deleteCustomerProductList: TODO('deleteCustomerProductList'), | ||
deleteCustomerProductList(customerId, {parameters}) { | ||
return { | ||
// TODO: Rather than invalidate, can we selectively update? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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... |
||
invalidate: [{queryKey: getCustomerProductLists.queryKey(parameters)}], | ||
remove: [{queryKey: getCustomerProductList.queryKey(parameters)}] | ||
bendvc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
}, | ||
deleteCustomerProductListItem(customerId, {parameters}) { | ||
return { | ||
// TODO: Rather than invalidate, can we selectively update? | ||
|
@@ -96,7 +97,6 @@ export const cacheUpdateMatrix: CacheUpdateMatrix<Client> = { | |
} | ||
}, | ||
getResetPasswordToken: noop, | ||
invalidateCustomerAuth: TODO('invalidateCustomerAuth'), | ||
// TODO: Should this update the `getCustomer` cache? | ||
registerCustomer: noop, | ||
// TODO: Implement when the endpoint exits closed beta. | ||
|
@@ -133,7 +133,13 @@ export const cacheUpdateMatrix: CacheUpdateMatrix<Client> = { | |
} | ||
}, | ||
updateCustomerPassword: noop, | ||
updateCustomerProductList: TODO('updateCustomerProductList'), | ||
updateCustomerProductList(customerId, {parameters}) { | ||
return { | ||
update: [{queryKey: getCustomerProductList.queryKey(parameters)}], | ||
// TODO: Rather than invalidate, can we selectively update? | ||
invalidate: [{queryKey: getCustomerProductLists.queryKey(parameters)}] | ||
} | ||
}, | ||
updateCustomerProductListItem(customerId, {parameters}) { | ||
return { | ||
update: [{queryKey: getCustomerProductListItem.queryKey(parameters)}], | ||
|
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