Account's wishlist: integrate hooks

[vmarta]

Progress so far:

• For editing a wishlist item: tweak the cache-update logic so it also selectively updates the entire lists data
• The wishlist page is now loading properly

What's next:

• Continue replacing more SCAPI calls, in particular the button-click event handlers
• Fix the failing tests

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

• (change1)

How to Test-Drive This PR

• (step1)

Checklists

General

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

Accessibility Compliance

You must check off all items in one of the follow two lists:

• There are no changes to UI

or...

• Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
• Changes comply with WCAG 2.0 guidelines levels A and AA
• Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

• Changes include a UI text update in the Retail React App (which requires translation)

[kevinxh]

JSON.parse has performance issues... Does the suggestion below work for typescript?

Suggested change

	const clone: ShopperCustomersTypes.CustomerProductListResult = JSON.parse(
	JSON.stringify(oldData)
	)
	const clone: ShopperCustomersTypes.CustomerProductListResult = {...oldData}

[vmarta]

I had {...oldData} before, but it was just a shallow clone. I wanted a deep clone, since the property to update is nested.

[wjhsf]

Here's the most concise (but maybe not the most readable) way to implement this logic. General idea is "Shallow clone the object, deep clone the array, but replace the array item with matching ID" for both the list of lists and for the list of items.

return {
  ...oldData,
  data: oldData.data.map((list) =>
    list.id !== listId
      ? list
      : {
        ...list,
        customerProductListItems:
          list.customerProductListItems?.map((item) =>
            item.id !== itemId ? item : response
          )
        }
  )
}

[vmarta]

❤️ Thanks Will! Not only it's more concise, but I think it's more readable too.

[wjhsf]

Here's the most concise (but maybe not the most readable) way to implement this logic. General idea is "Shallow clone the object, deep clone the array, but replace the array item with matching ID" for both the list of lists and for the list of items.

return {
  ...oldData,
  data: oldData.data.map((list) =>
    list.id !== listId
      ? list
      : {
        ...list,
        customerProductListItems:
          list.customerProductListItems?.map((item) =>
            item.id !== itemId ? item : response
          )
        }
  )
}

[wjhsf]

Don't forget to remove the query from the invalidate list; invalidate takes precedence over update. (Technically, it will update then invalidate, which negates the point of updating.)

[vmarta]

Actually, after the list item is updated, the product lists become somewhat stale. For example, the lastModified time is no longer true.

So I prefer to also invalidate the product lists. Yes, most of the data can be redundant, but the refetch is going to be done in the background. In some way, I think of it as similar to stale-while-revalidate strategy.

[wjhsf]

If we update and invalidate at the same time, does react query ever show the updated data? I thought that it just keeps the old data until the refetch completes. (I could very easily be wrong.)

Also, I just noticed that we update/invalidate the lists endpoint, but we just invalidate the list endpoint. We should follow the same pattern for both.

[vmarta]

You brought up good points there. To make things simpler, I've reverted the cache-update logic.

This article mentions that it's safer and simpler to just invalidate, rather than update. And I can see that here in our case.

So what now? There's actually another way to achieve what I wanted: via local state update in the quantity picker component. Take a look at this commit for details: 487c775

[kevinxh]

nice await!

[kevinxh]

why? I see that we previously do this but don't understand why, maybe it's worth adding a comment on this.

[vmarta]

It's so the callee can know when something has ended (and give you a chance to remove the loading spinner, for example). I've updated the code so it's clearer now.

[kevinxh]

I'm not entirely sure if i understand, it's really weird to the caller that when user clicks the button, onClick actually fires twice!

[vmarta]

🤔 maybe instead of onClick prop, we have 2 props?

• onActionStart
• onActionEnd

[vmarta]

@kevinxh All right, I've converted the onClick into those 2 events to be clearer.

[kevinxh]

sorry, i just saw this now... You could actually make onClick a promise.

[kevinxh]

more awaits! good stuff!

[wjhsf]

async with no await?

[wjhsf]

👏

[wjhsf]

This will send a request if productIds is an empty array. Is that desired behavior?

[vmarta]

No, I'll update it to check for an array and that it's not empty.

[wjhsf]

If we update and invalidate at the same time, does react query ever show the updated data? I thought that it just keeps the old data until the refetch completes. (I could very easily be wrong.)

Also, I just noticed that we update/invalidate the lists endpoint, but we just invalidate the list endpoint. We should follow the same pattern for both.

[adamraya]

The code in develop removes the item from the wishlist when the shopper decrements the product quantity < 1, when taking either of these two actions in the QuantityPicker:

1. Typing the number '0' in the quantity Input field.
2. Pressing the '-' button when the quantity is 1.

[vmarta]

Ok, the smart deletion logic originally comes from:

pwa-kit/packages/template-retail-react-app/app/commerce-api/hooks/useCustomerProductLists.js

Lines 204 to 210 in 487c775

	updateListItem: async (listId, item) => {
	const {id, quantity} = item
	if (quantity === 0) {
	await removeListItem(listId, id)
	actions.removeListItem(listId, id)
	return
	}

[vmarta]

FYI I've implemented that.

[adamraya]

On develop when the shopper clicks the "Add to Wishlist" button on one of the items of a product set, the individual item is added to the wishlist, whereas now the entire set is added to the wishlist instead of the individual product.

Example product set:
https://pwa-kit.mobify-storefront.com/global/en-GB/product/fall-lookM

[vmarta]

Hmm strange, that might be a regression after resolving merge conflicts.. let me investigate.

[vmarta]

Ok I've fixed this issue.

[wjhsf]

What happens if you do update with 0? Feels like the back end should be capable of handling the deletion logic when quantity is 0.

[vmarta]

What would happen is the back end would accept 0 as the minimum quantity, but it does not remove the item from the list.

[kevinxh]

nit, if onClick is a promise, then the consumer will know when the action ends. An example in our code base is the action card component's onRemove prop

[vmarta]

This is a bug fix for making sure that adding to wishlist works correctly for a product set: you can add the parent item to the wishlist or the individual child items.