-
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
Conversation
createShopperContext(customerId, {parameters}) { | ||
return { | ||
invalidate: [{queryKey: getShopperContext.queryKey(parameters)}] | ||
} | ||
}, | ||
updateShopperContext(_customerId, {parameters}, response) { | ||
return { | ||
update: [ | ||
{ | ||
queryKey: getShopperContext.queryKey(parameters), | ||
updater: (): ShopperContextsTypes.ShopperContext | undefined => ({...response}) | ||
} | ||
] | ||
} | ||
}, | ||
deleteShopperContext(_customerId, {parameters}) { | ||
return { | ||
remove: [{ | ||
queryKey: getShopperContext.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.
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.
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 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
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
- basket mutation: addToCart
- 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...
@@ -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 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).
|
||
if (cacheUpdates.clear) { | ||
queryClient.clear() | ||
} |
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 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.
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 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. 🤣
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.
@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 comment
The reason will be displayed to describe this comment to others. Learn more.
Looks ace! 🙌
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 ❗ comment)
@@ -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 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.
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.
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:
- 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.
- 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.
🤔
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.
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.
|
||
export const AuthHelpers = { | ||
LoginGuestUser: 'loginGuestUser', | ||
LoginRegisteredUserB2C: 'loginRegisteredUserB2C', | ||
Logout: 'logout', | ||
Register: 'register' |
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.
Why no more register
?
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 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.
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.
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.
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.
Looks ace! 🙌
} from 'commerce-sdk-react-preview' | ||
import Json from '../components/Json' | ||
|
||
const renderQueryHook = (name: string, {data, isLoading, error}: any) => { |
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 don't like any
(but I recognize that this isn't a super important place to avoid it).
{queryHooks.map(({name, hook}) => { | ||
return renderQueryHook(name, {...hook}) |
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.
Weird that name
is destructured to a separate variable, but everything else is left in the hook object (and name
not destructured in the mutations).
Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Will Harney <[email protected]>
…eCommerceCloud/pwa-kit into add-cache-update-logic
Co-authored-by: Will Harney <[email protected]>
packages/commerce-sdk-react/src/components/ShopperExperience/Page/index.tsx
Outdated
Show resolved
Hide resolved
logoutCustomer: TODO('logoutCustomer'), | ||
logoutCustomer: () => { | ||
return { | ||
remove: [{queryKey: ['/commerce-sdk-react']}] |
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.
Why a leading /
in the namespace? Query keys are arbitrary, they don't necessarily need to look like paths!
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.
packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.test.ts
Outdated
Show resolved
Hide resolved
} | ||
}, | ||
updateShopperContext(_customerId, {parameters}) { | ||
return { |
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
…test.ts Co-authored-by: Will Harney <[email protected]>
…age/index.tsx Co-authored-by: Will Harney <[email protected]>
…eCommerceCloud/pwa-kit into add-cache-update-logic
packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts
Outdated
Show resolved
Hide resolved
Co-authored-by: Adam Raya <[email protected]>
* Update `develop` with `release-v2.7.0` (#1033) * Starting release process for 2.7.0 * Update Create MRT credentials file (#1025) * Release 2.7: fix order of hooks (#1027) * Return early only after all of the hooks are called * Bumper version --------- Co-authored-by: Ben Chypak <[email protected]> * Quick fix for einstein data (#1028) * Quick fix for einstein data * Bump Version * Re-lock package-lock files with npm 8 * Update package-lock.json * Update einstein.js * Regenerate lock files (#1030) * Regenerate lock files * Bump version to 2.7.0-alpha.3 * Bump to 2.7.0 (#1032) * Begin development on 2.8.0 --------- Co-authored-by: Adam Raya <[email protected]> Co-authored-by: Vincent Marta <[email protected]> * Move the MRT reference app to the SDKs, so that we can verify eg. Node support (#966) * BUG: Changed type of the phone number field to bring up numberic keyboard on mobile devices - W-9871940 (#1016) Co-authored-by: Ben Chypak <[email protected]> * Update Retail React App Page Designer integration README (#1041) Co-authored-by: Richard Sexton <[email protected]> * Implement `updateCustomerPassword` as no-op. (#1031) * Allow query hook parameters to be `null`. (#1046) * Remove unused util. * Allow query hook parameters to be `null`. * Fix addresses not having preferred address first. (#1051) * Fix addresses not having preferred address first. * Include all addresses, not just preferred address twice. * Correctly include preferred address. * Make `mergeBasket` Conditional More Robust (#1048) * Update merge logic * Update CHANGELOG.md * Lint * PR feedback * Rename isNewlyRegisters to isNew for simplicity * Lint * [commerce-sdk-react] Decode pre-fetched token and save auth data in storage (#1052) * add test * decode jwt data * lint * Update packages/commerce-sdk-react/src/auth/index.ts Co-authored-by: Will Harney <[email protected]> * rename parseSlasJWT * Update packages/commerce-sdk-react/src/auth/index.test.ts Co-authored-by: Will Harney <[email protected]> * convert all .thens to await * make fake token in tests more clear * lint --------- Co-authored-by: Will Harney <[email protected]> * Prevent modal to open when it fails to add an item to cart (#1053) * prevent modal to open when it fails to add an item to cart * Update comment --------- Co-authored-by: Alex Vuong <[email protected]> * Fix `getConfig` referencing config from incorrect location (#1049) * Initial Commit * Update CHANGELOG.md * Revert some testing code * Get test coverage back up * Test build directory folder before proceeding. * fix(template-retail-react-app): product-list refinements (#957) * fix(template-retail-react-app): product-list refinements + Ensure that the `selectedFilters` provided to the `Refinements` value components is always an array. And update said value components (checkbox, color, radio & size) to handle always receiving an array. + Apply some loose equality checks, catering for cases where a refinement URL param is parsed as a number but the refinement `.value` is a stringified number. + Fix bug where non-multiple filters could not be unchecked by selecting themselves (e.g. clicking an already checked checkbox). + Fix bug where a refinement URL param is parsed as a number, but was handled as a string (with `.split()`). + Fix bug where radio refinements would still appear as checked after clearing the value via `SelectedRefinements`. + Deprecate unused proptypes in `SizeRefinements` * feat(retail-react-app): update `CheckboxRefinements` + Adjust the `onChange` logic so that multiple checkbox refinements can be selected --------- Co-authored-by: Ben Chypak <[email protected]> * update docs for shopper-experience scope (#1059) * add docs for experience scope * Add `merge_group` event to trigger GA builds --------- Co-authored-by: Will Harney <[email protected]> Co-authored-by: Adam Raya Navarro <[email protected]> * Update lockfiles to reflect current version. (#1071) * [commerce-sdk-react] Implement remaining Shopper Baskets cache logic (#1070) * Refactor Shopper Basket cache following new pattern * Fix types * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/cache.ts Co-authored-by: Will Harney <[email protected]> * PR Feedback * Clean up unused utils and types after refactor * Add missing response basketId to queryKey * Implement cache for the remaining mutations * Tests WIP * PR Feedback & Use query key path in `deleteBasket` cache * Add tests for mutations returning void response * PR Feedback * Remove unused `and` hooks util * Revert "Remove unused `and` hooks util" This reverts commit c0a364a. --------- Co-authored-by: Will Harney <[email protected]> * [commerce-sdk-react] Fix Shopper Baskets Test case (#1082) * Fix Shopper Basket empty response test cases * lint * feat(pwa-kit-dev): minor performance improvements and added comments (#974) * feat(pwa-kit-dev): minor performance improvements and added comments * docs(pwa-kit-dev): clean up comments * refactor(pwa-kit-dev): update condition from PR feedback --------- Co-authored-by: Will Harney <[email protected]> * Update dependencies. (#1079) * Remove internal packages to bypass lerna nonsense. Include output.json to make restoring deps easier. * Disable build scripts to make changing deps easier. * Update root deps. * Fix typescript issue. * Create restore script to restore internal deps/scripts. * Update commerce-sdk-react deps. * Update test-commerce-sdk-react deps. * Update template-typescript-minimal deps * Improve restore script. * Fix trailing comma. * Update template-retail-react-app deps * npm prune everything * Update pwa-kit-runtime deps * Update pwa-kit-react-sdk deps * Update pwa-kit-create-app deps * Update pwa-kit-dev deps (except eslint) * Update pwa-kit-dev eslint deps * Update internal-lib-build deps (except eslint) * Update pwa-kit eslint deps * Restore internal deps. * Restore build scripts. * Remove temporary helper files. * Bump ua-parser-js to avoid vulnerable version. * Anchor cross-env common dep to version used by internal-lib-build. * Re-enable eslint. * Implement Cache Logic for Shopper APIs (Contexts/Customers/Login/Orders) (#1073) * Initial commit * Update packages/commerce-sdk-react/src/hooks/useAuthHelper.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/hooks/ShopperOrders/cache.ts Co-authored-by: Will Harney <[email protected]> * Added root to all query keys, use remove over clear * Remove previous impemented "clear" from utils * Initial tests for shoppercontexts * Update ShopperLogin tests * Fix order tests * Update packages/commerce-sdk-react/src/hooks/ShopperContexts/cache.ts Co-authored-by: Will Harney <[email protected]> * Update cache.ts * Lint! * Update Json.tsx * Lint! * Testing race condition in tests * Re-add tests in other order. * Update CHANGELOG.md * Add todo to complete context cache work * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.test.ts Co-authored-by: Will Harney <[email protected]> * Update packages/commerce-sdk-react/src/components/ShopperExperience/Page/index.tsx Co-authored-by: Will Harney <[email protected]> * Update useAuthHelper.ts * Update packages/commerce-sdk-react/src/hooks/ShopperCustomers/cache.ts Co-authored-by: Adam Raya <[email protected]> --------- Co-authored-by: Will Harney <[email protected]> Co-authored-by: Adam Raya <[email protected]> * remove site alias and locale from location.state.directedFrom path (#1065) * remove site alias and locale from location.state.directedFrom path * moving call to removeSiteLocaleFromPath into use-navigation hook * fixing failing test, added tests for removeSiteLocaleFromPath * per code review, skipping failed test instead of using mocking * Fix Page Designer ImageWithText Link component (#1092) * Fix PD ImageWithText Link component by using Chakra UI Link * Use isAbsoluteURL to use react-router Link or Chakra Link component * PR Feedback * Clean up * Update packages/template-retail-react-app/app/page-designer/assets/image-with-text/index.jsx Co-authored-by: Ben Chypak <[email protected]> * Remove temporal page-viewer page to facilitate review --------- Co-authored-by: Ben Chypak <[email protected]> * split ssr build on local (#1155) * add suffix to ssr build files (#1157) * Added session bridge call to login for phased launch (#1159) * Added session bridge call to login for phased launch * Fix code smell for session-bridge in hybrid * Fix multi-value query params being lost (#1150) * Fix multi-value query params being lost * Update CHANGELOG.md * Snyk dependency updates (#1169) * Dependency updates * Update runtime package lock * Bump cosmiconfig version to latest * [Hybrid] PWA Kit should have a mechanism for replacing the access token when a SFRA login state is changed (#1171) * Implement mechanism to store refresh token copy and compare with sfra * Update tests and mocks for util function to check SFRA login state * Fix linting issues * FIx param types for util functionn * Rename old isTokenValid to isTokenExpired * Remove expiry for refresh_token in localstorage * Update packages/template-retail-react-app/app/commerce-api/utils.js Co-authored-by: Kevin He <[email protected]> * fix test * Fix linting on use-auth-modal.test.js * Update hasSFRAStateChanged logic to compare keys and values * Fix linting --------- Co-authored-by: Kevin He <[email protected]> * Add a redirect to login page after user signs out from checkout page (#1172) * Add a redirect to login page after user signs out from checkout page * Update CHANGELOG.md * Remove history since navigate handles similarly * Bump version number to 2.7.1-alpha.0 * Update changelogs * #1174 Replace invalid value for wrap property (#1179) * Update changelogs * Version bump to 2.7.1-preview.0 * Revert "Version bump to 2.7.1-preview.0" This reverts commit 985a7e0. * Update CHANGELOG.md * Rebuild lock files and fix ShopperLogin mutation test * Revert "Rebuild lock files and fix ShopperLogin mutation test" This reverts commit d9cfe50. * Add additional properties to ShopperLogin test types (#1185) * [V2] Re-generate lock files and fix hook lib tests (#1186) * re-generate lock files and fix test * Update packages/commerce-sdk-react/src/hooks/ShopperBaskets/index.test.ts * Rebuild lockfiles using node 14 npm 8 * Revert "Rebuild lockfiles using node 14 npm 8" This reverts commit 3d5c0cb. * Use pwa-kit-dev for lint and format * Revert "Use pwa-kit-dev for lint and format" This reverts commit f46d83e. * Add typescript to internal-lib-build and rebuild lock files --------- Co-authored-by: vcua-mobify <[email protected]> Co-authored-by: vcua-mobify <[email protected]> * [V2] Internal lib build typescript dev dependency (#1194) * Move typescript to dev and peer dependency * Update package lock file * Lockfile updates --------- Co-authored-by: Ben Chypak <[email protected]> Co-authored-by: Adam Raya <[email protected]> Co-authored-by: Vincent Marta <[email protected]> Co-authored-by: Oliver Brook <[email protected]> Co-authored-by: echessman <[email protected]> Co-authored-by: John Boxall <[email protected]> Co-authored-by: Richard Sexton <[email protected]> Co-authored-by: Will Harney <[email protected]> Co-authored-by: Kevin He <[email protected]> Co-authored-by: Alex Vuong <[email protected]> Co-authored-by: Brad Adams <[email protected]> Co-authored-by: Charles Lavery <[email protected]> Co-authored-by: Adam Raya Navarro <[email protected]> Co-authored-by: ecRobertEngel <[email protected]> Co-authored-by: Sandra Golden <[email protected]> Co-authored-by: Jainam Sheth <[email protected]> Co-authored-by: mdenchev-aiopsgroup <[email protected]>
Description
GUS: W-12665242
This PR implements the remaining cache matrix actions for the API's mentioned in the title. To fulfill the issues requirements it was necessary to add the cache matrix functionality to the auth
helpers
code as it wasn't present previously.Types of Changes
Changes
How to Test-Drive This PR
Checklists
General
TODO