-
Notifications
You must be signed in to change notification settings - Fork 146
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 1 commit
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 |
---|---|---|
|
@@ -4,17 +4,33 @@ | |
* SPDX-License-Identifier: BSD-3-Clause | ||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
import {ShopperContextsTypes} from 'commerce-sdk-isomorphic' | ||
import {ApiClients, CacheUpdateMatrix} from '../types' | ||
import {getShopperContext} from './queryKeyHelpers' | ||
|
||
type Client = ApiClients['shopperContexts'] | ||
|
||
/** 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 | ||
} | ||
export const cacheUpdateMatrix: CacheUpdateMatrix<Client> = { | ||
updateShopperContext: TODO('updateShopperContext'), | ||
createShopperContext: TODO('createShopperContext'), | ||
deleteShopperContext: TODO('deleteShopperContext') | ||
createShopperContext(customerId, {parameters}) { | ||
return { | ||
invalidate: [{queryKey: getShopperContext.queryKey(parameters)}] | ||
} | ||
}, | ||
updateShopperContext(_customerId, {parameters}, response) { | ||
return { | ||
update: [ | ||
{ | ||
queryKey: getShopperContext.queryKey(parameters), | ||
updater: (): ShopperContextsTypes.ShopperContext | undefined => ({...response}) | ||
bendvc marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
] | ||
} | ||
}, | ||
deleteShopperContext(_customerId, {parameters}) { | ||
return { | ||
remove: [{ | ||
queryKey: getShopperContext.queryKey(parameters) | ||
}] | ||
} | ||
} | ||
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. Shopper context is a special exception to how all the other endpoints work. Essentially you are manipulating a singleton on the backend, your context. One interesting side effect of the work her is that if you create the context singleton and then delete it, if you have an active hook to get the context it will run after the deletion and get a 404. I'm still investigating why this is happening, but could use some input. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -84,7 +84,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 +102,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 +138,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)}], | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,6 +192,7 @@ export type CacheUpdate = { | |
update?: CacheUpdateUpdate<unknown>[] | ||
invalidate?: CacheUpdateInvalidate[] | ||
remove?: CacheUpdateRemove[] | ||
clear?: boolean | ||
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. This is a new cache update action. If this is set to true, the entire cache is cleared. It was necessary to do this here as there was no access to the queryClient elsewhere (like the updater function). |
||
} | ||
|
||
/** Generates a collection of cache updates to make for a given request. */ | ||
|
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. @wjhsf I was able to scratch something together for the cache matrix in the auth helpers. I'll defo need your feedback on all the typescriptisms. 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. Looks ace! 🙌 |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -4,19 +4,30 @@ | |||||
* SPDX-License-Identifier: BSD-3-Clause | ||||||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||||||
*/ | ||||||
import {MutationFunction, useMutation, UseMutationResult} from '@tanstack/react-query' | ||||||
import {MutationFunction, useMutation, UseMutationResult, useQueryClient} from '@tanstack/react-query' | ||||||
import useAuthContext from './useAuthContext' | ||||||
import Auth from '../auth' | ||||||
import {Argument, CacheUpdate} from './types' | ||||||
import {updateCache} from './utils' | ||||||
|
||||||
export const AuthHelpers = { | ||||||
LoginGuestUser: 'loginGuestUser', | ||||||
LoginRegisteredUserB2C: 'loginRegisteredUserB2C', | ||||||
Logout: 'logout', | ||||||
Register: 'register' | ||||||
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. Why no more 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. I talked to @kevinxh about this. 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. Sorry, actually I was wrong! We are using the
The register helper is special, because it is not a ShopperLogin method, but rather a ShopperCustomer method. |
||||||
Logout: 'logout' | ||||||
} as const | ||||||
|
||||||
export type AuthHelper = (typeof AuthHelpers)[keyof typeof AuthHelpers] | ||||||
|
||||||
const noop = () => ({}) | ||||||
|
||||||
type CacheUpdateMatrix = { | ||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||
bendvc marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
[Method in AuthHelper]?: ( | ||||||
options: Argument<Auth[Method]> | void, | ||||||
response: ReturnType<Auth[Method]> extends Promise<infer D> ? D : never | ||||||
) => CacheUpdate | ||||||
} | ||||||
|
||||||
/** | ||||||
* A hook for Public Client OAuth helpers. | ||||||
* The hook calls the SLAS helpers imported from commerce-sdk-isomorphic. | ||||||
|
@@ -40,13 +51,32 @@ export function useAuthHelper<Mutation extends AuthHelper>( | |||||
> { | ||||||
const auth = useAuthContext() | ||||||
if (!auth[mutation]) throw new Error(`Unknown login helper mutation: ${mutation}`) | ||||||
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.
Suggested change
Just noticed we missed a spot when renaming the helpers. |
||||||
|
||||||
const queryClient = useQueryClient() | ||||||
// I'm not sure if there's a way to avoid this type assertion, but, I'm fairly confident that | ||||||
// it is safe to do, as it seems to be simply re-asserting what we already know. | ||||||
type Method = Auth[Mutation] | ||||||
type PromisedData = ReturnType<Method> | ||||||
type Data = PromisedData extends Promise<infer D> ? D : never | ||||||
type Variables = [] extends Parameters<Method> ? void : Parameters<Method>[0] | ||||||
const method = auth[mutation].bind(auth) as MutationFunction<Data, Variables> | ||||||
return useMutation(auth.whenReady(method)) | ||||||
return useMutation(auth.whenReady(method), { | ||||||
onSuccess(data, options) { | ||||||
const getCacheUpdates = cacheUpdateMatrix[mutation] | ||||||
|
||||||
if (getCacheUpdates) { | ||||||
const cacheUpdates = getCacheUpdates(options, data) | ||||||
updateCache(queryClient, cacheUpdates, data) | ||||||
} | ||||||
} | ||||||
}) | ||||||
} | ||||||
|
||||||
const cacheUpdateMatrix: CacheUpdateMatrix = { | ||||||
loginRegisteredUserB2C: noop, | ||||||
loginGuestUser: noop, | ||||||
wjhsf marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
logout() { | ||||||
return { | ||||||
clear: true | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,6 +31,10 @@ export const updateCache = (queryClient: QueryClient, cacheUpdates: CacheUpdate, | |
// If an updater isn't given, fall back to just setting the data | ||
queryClient.setQueryData(queryKey, updater ?? data) | ||
) | ||
|
||
if (cacheUpdates.clear) { | ||
queryClient.clear() | ||
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. ❗ The query cache is a global state. If end users are using react query for their own cache management, this will wipe out their data, too. Proposal: namespace our query keys (e.g. Adding a namespace is probably out of scope, here, but I think we can move in the right direction by doing 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. Thats a very good call out. I wasn't thinking about custom hooks that are used outside of the hooks library. I did however think about using So 2 questions here:
🤔 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. UPDATE: We decided to remove the "clear" functionality in favour of using "remove" since we don't want to effect any external cache since the client is potentially shared. Also, we added a namespace to all the cache keys to better avoid collisions. |
||
} | ||
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. This action intentionally happens after all the others if for some reason there are other updater actions defined alongside of the clear action, which shouldn't ever be the case. |
||
} | ||
|
||
/** Error thrown when a method is not implemented. */ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,13 +4,42 @@ | |
* SPDX-License-Identifier: BSD-3-Clause | ||
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause | ||
*/ | ||
import React from 'react' | ||
import React, {useState} from 'react' | ||
|
||
const Json = React.memo(({data}: {data: any}) => ( | ||
<div> | ||
<pre>{JSON.stringify(data, null, 2)}</pre> | ||
</div> | ||
)) | ||
const Json = ({data}: {data: any}) => { | ||
const [expanded, setExpanded] = useState(false) | ||
|
||
const style = { | ||
body: { | ||
position: 'relative', | ||
height: expanded ? 'inherit' : '100px', | ||
overflow: 'hidden' | ||
}, | ||
button: { | ||
position: 'absolute', | ||
right: 0, | ||
paddingRight: 10 | ||
}, | ||
shadow: { | ||
position: 'absolute', | ||
bottom: 0, | ||
left: 0, | ||
width: '100%', | ||
height: 10, | ||
background: | ||
'linear-gradient(to bottom, rgba(137,255,241,0) 0%,rgba(255,255,255,1) 100%)' | ||
} | ||
} | ||
return ( | ||
<div style={style.body}> | ||
<a style={style.button} onClick={() => setExpanded(!expanded)}> | ||
{expanded ? '[-]' : '[+]'} | ||
</a> | ||
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. I was tired of scrolling through all the response objects. This made things less infuriating. 🤣 |
||
<pre>{JSON.stringify(data, null, 2)}</pre> | ||
{!expanded && <div style={style.shadow} />} | ||
</div> | ||
) | ||
} | ||
|
||
Json.displayName = 'Json' | ||
|
||
|
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