Skip to content
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

Merged
merged 27 commits into from
Mar 24, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
00c402a
Initial commit
bendvc Mar 20, 2023
97b0190
Update packages/commerce-sdk-react/src/hooks/useAuthHelper.ts
bendvc Mar 21, 2023
20dfc21
Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts
bendvc Mar 21, 2023
5dc64b5
Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts
bendvc Mar 21, 2023
b3a55a8
Added root to all query keys, use remove over clear
bendvc Mar 22, 2023
c85a14a
Merge branch 'add-cache-update-logic' of https://github.com/Salesforc…
bendvc Mar 22, 2023
3d9002e
Remove previous impemented "clear" from utils
bendvc Mar 22, 2023
47df510
Initial tests for shoppercontexts
bendvc Mar 23, 2023
97359e3
Update ShopperLogin tests
bendvc Mar 23, 2023
d04745b
Fix order tests
bendvc Mar 23, 2023
f20ef61
Update packages/commerce-sdk-react/src/hooks/ShopperContexts/cache.ts
bendvc Mar 23, 2023
522196f
Update cache.ts
bendvc Mar 23, 2023
9a950fc
Lint!
bendvc Mar 23, 2023
e7bd07c
Merge branch 'develop' into add-cache-update-logic
bendvc Mar 23, 2023
a4b9191
Update Json.tsx
bendvc Mar 23, 2023
c685f96
Merge branch 'develop' into add-cache-update-logic
bendvc Mar 23, 2023
4b467a2
Lint!
bendvc Mar 23, 2023
1e83f14
Testing race condition in tests
bendvc Mar 23, 2023
5835c4d
Re-add tests in other order.
bendvc Mar 23, 2023
d378615
Merge branch 'develop' into add-cache-update-logic
bendvc Mar 23, 2023
d15b5de
Update CHANGELOG.md
bendvc Mar 23, 2023
42c1fe1
Add todo to complete context cache work
bendvc Mar 24, 2023
d146e2f
Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.…
bendvc Mar 24, 2023
2f6f01a
Update packages/commerce-sdk-react/src/components/ShopperExperience/P…
bendvc Mar 24, 2023
0d2b555
Merge branch 'add-cache-update-logic' of https://github.com/Salesforc…
bendvc Mar 24, 2023
e4ec495
Update useAuthHelper.ts
bendvc Mar 24, 2023
2229c09
Update packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts
bendvc Mar 24, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions packages/commerce-sdk-react/src/hooks/ShopperContexts/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

update: [
{
queryKey: getShopperContext.queryKey(parameters),
updater: (): ShopperContextsTypes.ShopperContext | undefined => ({...response})
}
]
}
},
deleteShopperContext(_customerId, {parameters}) {
return {
remove: [{
queryKey: getShopperContext.queryKey(parameters)
}]
}
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

}
17 changes: 14 additions & 3 deletions packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Copy link
Collaborator Author

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.

Copy link
Contributor

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.

Copy link
Collaborator

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

invalidation is a must have and selective updates are nice to haves

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

  1. basket mutation: addToCart
  2. customer query: getCustomerBaskets

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)}]
}
},
deleteCustomerProductListItem(customerId, {parameters}) {
return {
// TODO: Rather than invalidate, can we selectively update?
Expand All @@ -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.
Expand Down Expand Up @@ -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)}],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ const TODO = (method: string): undefined => {

export const cacheUpdateMatrix: CacheUpdateMatrix<ApiClients['shopperLogin']> = {
authorizePasswordlessCustomer: noop,
logoutCustomer: TODO('logoutCustomer'),
logoutCustomer: () => {
return {
clear: true
}
},
getAccessToken: noop,
getSessionBridgeAccessToken: noop,
getTrustedSystemAccessToken: noop,
Expand Down
18 changes: 9 additions & 9 deletions packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/BSD-3-Clause
*/
import {getCustomerBaskets} from '../ShopperCustomers/queryKeyHelpers'
import {ApiClients, CacheUpdateMatrix, CacheUpdateUpdate, CacheUpdateInvalidate} from '../types'
import {ApiClients, Argument, CacheUpdateMatrix, CacheUpdate, CacheUpdateUpdate, CacheUpdateInvalidate, MergedOptions} from '../types'
import {getOrder} from './queryKeyHelpers'

type Client = ApiClients['shopperOrders']
/** Parameters that get passed around, includes client config and possible parameters from other endpoints */
type GetOrderOptions = MergedOptions<Client, Argument<Client['getOrder']>>

/** 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
}
const invalidateOrderQuery = (customerId: string | null, {parameters}: GetOrderOptions): CacheUpdate => ({
invalidate: [{queryKey: getOrder.queryKey(parameters)}]
})

export const cacheUpdateMatrix: CacheUpdateMatrix<Client> = {
createOrder(customerId, {parameters}, response) {
Expand All @@ -31,7 +31,7 @@ export const cacheUpdateMatrix: CacheUpdateMatrix<Client> = {
: [{queryKey: getCustomerBaskets.queryKey({...parameters, customerId})}]
return {update, invalidate}
},
createPaymentInstrumentForOrder: TODO('createPaymentInstrumentForOrder'),
removePaymentInstrumentFromOrder: TODO('removePaymentInstrumentFromOrder'),
updatePaymentInstrumentForOrder: TODO('updatePaymentInstrumentForOrder')
createPaymentInstrumentForOrder: invalidateOrderQuery,
removePaymentInstrumentFromOrder: invalidateOrderQuery,
updatePaymentInstrumentForOrder: invalidateOrderQuery
}
1 change: 1 addition & 0 deletions packages/commerce-sdk-react/src/hooks/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -192,6 +192,7 @@ export type CacheUpdate = {
update?: CacheUpdateUpdate<unknown>[]
invalidate?: CacheUpdateInvalidate[]
remove?: CacheUpdateRemove[]
clear?: boolean
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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. */
Expand Down
39 changes: 34 additions & 5 deletions packages/commerce-sdk-react/src/hooks/useAuthHelper.ts
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks ace! 🙌

Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,29 @@
* 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'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no more register?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I talked to @kevinxh about this. register doesn't exist in the helper namespace, so I reached out to Kev an he said I could delete it. It looks like it wasn't being used either.

Copy link
Collaborator

@kevinxh kevinxh Mar 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, actually I was wrong! We are using the register method in the retail react app.

And register method was lost in the big previous refactor.

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 = {
[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.
Expand All @@ -40,13 +50,32 @@ export function useAuthHelper<Mutation extends AuthHelper>(
> {
const auth = useAuthContext()
if (!auth[mutation]) throw new Error(`Unknown login helper mutation: ${mutation}`)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (!auth[mutation]) throw new Error(`Unknown login helper mutation: ${mutation}`)
if (!auth[mutation]) throw new Error(`Unknown auth helper: ${mutation}`)

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,
logout() {
return {
clear: true
}
}
}
4 changes: 4 additions & 0 deletions packages/commerce-sdk-react/src/hooks/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Contributor

Choose a reason for hiding this comment

The 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. ['commerce-sdk-react', ...path, parameters]) and then do a remove that just matches our namespace (e.g. remove: {queryKey: ['commerce-sdk-react']}).

Adding a namespace is probably out of scope, here, but I think we can move in the right direction by doing remove with an empty array (I think that matches everything?). That would approximate clear, while setting us up to easily come back later and add the namespace.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 remove as a way to clear the cache using the /organizations/ query key partial. But had opted not to go that route was the implementation within react-query suggests that clear would be more performant. I believe remove will do batching and remove cache in chunks.

So 2 questions here:

  1. Do we really care about how fast removing of the cache is? if you think about it, someone can just close their browser and leave all the cache in there anyway.
  2. Should we care about unintentionally clearing cache that is outside of the commerce-sdk? I think the usecases are slim, but I suppose there are potential for wanting that behaviour. E.g. Someone has a react app that has a single click buy now button for a product, where it will login as a guest, add item to a cart, checkout, logout. We wouldn't want their external react query cache to be cleared in that case.

🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.

}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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. */
Expand Down
41 changes: 35 additions & 6 deletions packages/test-commerce-sdk-react/app/components/Json.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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'

Expand Down
3 changes: 3 additions & 0 deletions packages/test-commerce-sdk-react/app/pages/home.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@ const Home = () => {
<li>
<Link to="/orders">useShopperOrders & useShopperOrdersMutation</Link>
</li>
<li>
<Link to="/context">useShopperContext & useShopperContextsMutation</Link>
</li>
</ul>

<h2>Miscellaneous</h2>
Expand Down
Loading