Major wishlist refactor

[kevinxh]

Note: This PR introduces major changes to wishlist implementation (if not a re-write)...

While working on two wishlist bugs, I realize the wishlist implementation has a few fundamental issues that causes the code to be fragile and if we don't fix the issues now, there will be endless problem coming our way. Here is a summary of the issues:

Bad API design (separation of concerns)

The initial implementation uses a hook useCustomerProductLists for wishlist, the APIs of this hook is badly designed so the module internal knowledge (should stayed in the hook) is leaked to the pages that consume this hook.

snippet example from PDP:

    const addItemToWishlist = async (quantity) => {
        try {
            // If product-lists have not loaded we push "Add to wishlist" event to eventQueue to be
            // processed once the product-lists have loaded.

            // @TODO: move the logic to useCustomerProductLists
            // PDP shouldn't need to know the implementation detail of the event queue
            // PDP should just do "customerProductLists.addItem(item)"!
            if (!customerProductLists?.loaded) {
                const event = {
                    item: {...product, quantity},
                    action: 'add',
                    listType: customerProductListTypes.WISHLIST
                }

                customerProductLists.addActionToEventQueue(event)
            } else {
                const wishlist = customerProductLists.getProductListPerType(
                    customerProductListTypes.WISHLIST
                )

                const productListItem = wishlist.customerProductListItems.find(
                    ({productId}) => productId === product.id
                )
                // if the product already exists in wishlist, update the quantity
                if (productListItem) {
                    await customerProductLists.updateCustomerProductListItem(wishlist, {
                        ...productListItem,
                        quantity: productListItem.quantity + parseInt(quantity)
                    })
                    showWishlistItemAdded(quantity)
                } else {
                    // other wise, just create a new product list item with given quantity number
                    await customerProductLists.createCustomerProductListItem(wishlist, {
                        productId: product.id,
                        priority: 1,
                        quantity,
                        public: false,
                        type: 'product'
                    })
                    showWishlistItemAdded(quantity)
                }
            }
        } catch (error) {
            showError(error)
        }
    }

If you look closely in the above example, customerProductLists.addActionToEventQueue is implemented as a public API of the hook, this is bad for separation of concern because PDP only needs invoke an API method to add an item to the wishlist, the queue is a implementation detail that should be handled inside the hook. Unfortunately we choose to expose the queue to the outside world as oppose to encapsulate the queue logic inside the hook.

In this PR, the same functionality in PDP is simplified to:

const handleAddToWishlist = async (quantity) => {
        try {
            await createWishlistItem({
                id: product.id,
                quantity
            })
        } catch (e) {
            showError(e)
        }
    }

Customer Product Lists v.s. Wishlist

We've implemented the useCustomerProductLists hook with a piece of mind that the code should be generic, the Commerce API product list endpoint is designed in a way that allow shoppers to create multiple lists. So we've implemented the hook methods to have a 1:1 relationship with the API endpoints. While this is right, we should also be thinking how do we implement this feature that also provides easy-to-use methods for the "single wishlist" design in the retail react app.

In this PR, a new useWishlist hook is added, it is built on top of the useCustomerProductLists hook to provide wishlist specific functionalities. It inherits the customer product list methods and you can think of useWishlist as the child class.

Therefore, you have the best of both worlds - if your project is like the retail react app where there is only 1 single wishlist, you use useWishlist; if you want to manage multiple product lists, you use useCustomerProductLists.

Which list is the actual Wishlist???

Previously, the wishlist logic is implemented in a non-deterministic way. The hook chooses a random list from multiple customer product lists and use that as the wishlist.

The code used to determine which customer product list is the wishlist:

getProductListPerType(type) {
  return customerProductLists?.data.find((list) => list.type === type)
},

The logic chooses the first wish_list type list from all customer product lists as the wishlist. The outcome is solely depends on the sort order from the API and there is no deterministic sorting for the API endpoint according to the documentation, so this is pretty fragile! Imagine the customer adds a new product list from a different channel and the order of the list is changed. The shopper will be using a different list from the last time he logged in.

I propose that we use the name field to determine a specific list to use for the Retail React app, for example, a list called PWA wishlist, it is managed by the useWishlist hook, so it is guaranteed to always use the same list.

Wishlist page does NOT fetch wishlist

Guess what, the wishlist page doesn't fetch wishlist 😅 . The wishlist fetching logic is added to the top level useShopper hook.

First problem with the fetching logic is that, It always fetch all product lists + all products from every single list (O(n^2)). Where as we should only fetch all lists + products from just the wishlist (O(n)). (API limitation: there is no way to just fetch a single list).

Second problem: any changes to the product lists will trigger the code to re-fetch everything, this is bad for performance.

How to test-drive this PR

Since this is a major refactoring, i'd like the reviewer to help me to test the heck out of this.

You want to make sure:

• wishlist page works, including loading items, removing items, updating items, add wishlist item to the cart.
• you can add items to wishlist from PDP, PLP
• on PLP, the wishlist items are marked with the solid heart icon and can be removed from wishlist
• on cart page, you should be able to add item to the wishlist
• performance wise, look closely at the network tab when you test the above features, make sure there are no excessive calls
• log out, and add item to wishlist, verify that after user logs in, the item is then automatically added to wishlist

Changes

• move commerce api context from utils to its own file context.js
• re-write useCustomerProductList hook and remove the event queue
• move wishlist initialization out from useShopper and move to app component
• create useWishlist hook in user land to make code cleaner

Known Issue

1. DRY the toasts!
   The wishlist logic is mostly extracted into the hooks, ideally the toast logic should also be in there, but there is a limitation that intl provider is not available on top level hooks and we can't move toast logic into the hook at the moment. So you will see toast duplicated on PDP, PLP, cart, wishlist pages.

2. hard refresh on wishlist page will result in 2 dup requests sent to the product-list endpoint, that's because both the app and wishlist page tries to make that request, one potential solution might be to come up with a request caching strategy.

General

• Add a high-level description of your changes to relevant CHANGELOG.md files (not required for doc updates)
• The code changes are covered by test cases
• Analytics events instrumented to measure impact of change

Backwards Compatibility

• This PR contains no breaking changes or breaking changes have been approved by Product Management/Engineering as necessary.

Breaking changes include:

• Removing a public function/Component/Prop
• Adding required arguments to functions
• Changing the data-types of function params or return values

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

Documentation

• If you've updated documentation, have someone review it using the documentation reviewer's checklist

Documentation and comments should follow the Mobify Content Style Guide and Mobify's Capitalization and Spelling List

[kevinxh]

:( added a bunch of code and now the bundle size sits at 39.26kb...

[bendvc]

The blur for this utility says that the function passed in should be async. It's probably worth testing the function and error'ing out if it's not.

Something like this:

if (func.constructor.name !== `AsyncFunction`) {
    throw new Error('Parameter func must be asynchronous')
}

[kevinxh]

sorry my comment is misleading and I corrected it.

we are good as long as the func is a function and it returns a promise, it doesn't have to use the syntactic suger async.

[kevinxh]

i'll add some tests

[bendvc]

The function you are passing into handleAsyncError are not asynchronous, I'm not sure what kind of errors this is causing, but it's probably not functioning correctly as you are awaiting on a sync function, but it will execute immediately because the api call is async. You probably want to have async declared before the function parentheses.

[kevinxh]

sorry my jsdoc comment is misleading and I corrected it.

we are good as long as the func is a function and it returns a promise, it doesn't have to use the syntactic suger async.

[bendvc]

What is the purpose of doing for sectioning off these functions? Are we expecting that the end user will want to use the "requests" on their own outside of the stateful environment of the hook? Or is it purely stylistic/organizational?

My concern is that this hook already different that all the others with respects to using reducers, but now the hook it getting organized differently.

Also if you looks at the hypothetical call stack for using this hook it looks like, I'm going to call something like "createList" on the hook, that will call something like "createList" but it's a request and then after that an action with the name similar to "createList" will be called. This is all a bit confusing.

[kevinxh]

let's chat about this one