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

[V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer #1113

Merged
merged 25 commits into from
Apr 11, 2023

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Apr 5, 2023

Description

In this PR we leverage the ability to proactively update cache without having to invalidate and refetch what we already have. For example, when we create and post a new customer address, we are returned the customer object with the new address assigned to it. Instead of invalidating the current cache for the customer, which would in turn cause a fetch to get new data, we can take the updated customer and update the cache client side.

We do this throughout the ShopperCustomer endpoints, below is a list of when we manually update cache:

  • Create/Update/Delete CustomerAddresses
  • Create/Update/Delete ProductLists
  • Create/Update/Delete ProductListItems
  • Create/Update/Delete CustomerPaymentInstruments
  • Update Customer

NOTES

  • I added a new lib called clone as to not mutate the values passed to updater methods. They might have observers attached to them.
  • Right now I am bailing when there is no cache item to be updated, for example if you add an address to a known customer and you have no previously loaded said customer the "oldData" value for the customer updater would be undefined. Since the response has the full customer, we can proactively update the cache with the its first value. I'm working on this.
  • I'm working on DRY-ing up the code, right there is a lot of duplication.

Clone 3PP

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

Breaking changes include:

  • Removing a public function or component or prop
  • Adding a required argument to a function
  • Changing the data type of a function parameter or return value
  • Adding a new peer dependency to package.json

Changes

  • Update cache matrix for shopper customer
  • Update and add missing tests

How to Test-Drive This PR

  • Run tests in commerce-sdk-react

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

@bendvc bendvc marked this pull request as ready for review April 6, 2023 22:07
@bendvc bendvc requested a review from a team as a code owner April 6, 2023 22:07
* @param update
* @returns
*/
const createUpdateFunction =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open to feedback on these types and this function generator @wjhsf

@bendvc bendvc requested review from kevinxh and wjhsf April 6, 2023 22:10
Copy link
Contributor

@wjhsf wjhsf left a comment

Choose a reason for hiding this comment

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

There's a lot of repeated patterns, so most of my comments apply to multiple places. But I've only left them once.

{queryKey: getCustomerProductLists.queryKey(parameters)}
]
// We can only update cache for this product list item if we have the ID
// QUESTION: Why would we get a response that doesn't have an ID?
Copy link
Contributor

Choose a reason for hiding this comment

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

The RAML spec does not guarantee that it will be present. You'll have to ask the API team about that.

bendvc and others added 9 commits April 10, 2023 09:19
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
@@ -12,7 +12,7 @@

/* Language and Environment */
"target": "es2016" /* Set the JavaScript language version for emitted JavaScript and include compatible library declarations. */,
// "lib": [], /* Specify a set of bundled library declaration files that describe the target runtime environment. */
"lib": ["dom", "es2019"], /* Specify a set of bundled library declaration files that describe the target runtime environment. */
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have target: es2016, should we also use the es2016 lib, so that we only use available features? Or are we fine with the compiler inserting things that we need?

Copy link
Collaborator Author

@bendvc bendvc Apr 10, 2023

Choose a reason for hiding this comment

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

From what I read there is a kind of chaining that would be happening es2019 uses es2018 and it goes down the line. Anyway. I needed to add this to get the deep clone implementation working using the entries method. In general I'm not 100% sure if the solution is 100% correct. But it works as is, so maybe revisit it as needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fairly confident that this is fine from a "code that compiles and runs" standpoint. The thing I'm not sure about is how the es2019 code gets changed to es2016 code, particularly the impact on bundle size. Might be worth investigating as a separate ticket. (Outcome would probably either be "this is fine" or "don't use modern code so we don't add polyfill bloat".)

bendvc and others added 2 commits April 10, 2023 14:47
Co-authored-by: Will Harney <62956339+wjhsf@users.noreply.github.com>
kevinxh
kevinxh previously approved these changes Apr 10, 2023
Comment on lines 113 to +118
const itemId = response.id
if (!itemId) return {invalidate}

return {
invalidate,
update: [{queryKey: getCustomerProductListItem.queryKey({...parameters, itemId})}]
update: [
{
queryKey: getCustomerProductListItem.queryKey({...parameters, itemId})
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the RAML spec, we could get a response with no id, meaning this update could potentially be storing data in the cache that we never actually retrieve (itemId: undefined).

In practice, though, I don't know when we'd ever not receive an id.

@bendvc bendvc merged commit 056b669 into v3 Apr 11, 2023
bfeister added a commit that referenced this pull request Apr 25, 2023
* v3:
  [Spike]Replace watch with nodemon (#1146)
  Fix Page Designer ImageWithText Link component (#1092) (#1148)
  Upgrade deprecated dependencies (#1124)
  Restart login flow if refresh_token is invalid (#1135)
  Move some util to site-utils to avoid circular imports (#1133)
  Restore old file name. (#1140)
  Update eslint configuration (#1129)
  CI: clarify the environment variables (#1127)
  Remove react-query-devtools from production build (#1121)
  Update lerna.json (#1118)
  Parallelize lighthouse ci (#1126)
  Increase test timeouts only on CI env (#1123)
  Remove unused `request` deprecated dependency. (#1125)
  Upgrade msw to latest (#1100)
  Add mergeBasket hook (#1114)
  [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113)
  dont use callback on mutateAsync (#1119)
  2-spaces not 4-spaces (#1117)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-dev/src/configs/webpack/config.js
#	packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/template-express-minimal/package-lock.json
#	packages/template-mrt-reference-app/package-lock.json
#	packages/template-retail-react-app/package-lock.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/test-commerce-sdk-react/package-lock.json
bfeister added a commit that referenced this pull request Apr 25, 2023
…-rehaul

* feature/template-extensibility:
  [Spike]Replace watch with nodemon (#1146)
  Fix Page Designer ImageWithText Link component (#1092) (#1148)
  Upgrade deprecated dependencies (#1124)
  Restart login flow if refresh_token is invalid (#1135)
  Move some util to site-utils to avoid circular imports (#1133)
  Restore old file name. (#1140)
  Update eslint configuration (#1129)
  CI: clarify the environment variables (#1127)
  Remove react-query-devtools from production build (#1121)
  Update lerna.json (#1118)
  Parallelize lighthouse ci (#1126)
  Increase test timeouts only on CI env (#1123)
  Remove unused `request` deprecated dependency. (#1125)
  Upgrade msw to latest (#1100)
  Add mergeBasket hook (#1114)
  [V3][Hooks Integration 🪝] Manually update cache for ShopperCustomer (#1113)
  dont use callback on mutateAsync (#1119)
  2-spaces not 4-spaces (#1117)

# Conflicts:
#	packages/internal-lib-build/package-lock.json
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-runtime/package-lock.json
#	packages/pwa-kit-runtime/package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants