fix: no more infinite error loop if cart creation fails

[sirugh]

Description

Cart creation is currently handled by two actions. getCartDetails which calls createCart if there is no cart id when the app initializes. Unfortunately, if there is an error during the createCart action, we only dispatched the error (which reset the cart slice to initial state where cartId === null) and then retried the getCartDetails action. This results in an infinite loop.

The fix is to allow createCart to throw, but to gracefully handle the error and just return from getCartDetails which allows the user to continue browsing the site or doing whatever else they want, so long as it doesn't involve a cart.

I had considered popping a toast for this, but that gets into territory I'd rather leave for later. We should always have a valid cart when we try to perform some action that requires one, but we shouldn't prevent a user from browsing the site if creation failed.

Related Issue

Closes PWA-732.
Closes #2522

Acceptance

Verification Stakeholders

Specification

Verification Steps

1. In chrome dev tools, block the /graphql endpoint.
2. Clear cache and refresh the page.
3. You might see a lot of network failures, but it should not infinitely request.

Screenshots / Screen Captures (if appropriate)

Checklist

• I have added tests to cover my changes, if necessary.

• I have updated the documentation accordingly, if necessary.

[PWAStudioBot]

	Fails
🚫	A version label is required. A maintainer must add one.

	Messages
📖	Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.
📖	DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).

Generated by 🚫 dangerJS against de6bd81

[sirugh]

This feels a bit rough but also necessary. The retry logic in other actions necessitate that we somehow inform them of an unrecoverable error. I have chosen to throw in the root action and catch upstream but I wonder if there is a better way to do it. Perhaps propagate the error to the store and then, if an error such as that is present at the start of an action, bail?

// reducers/cart.js
const reducerMap = {
    [actions.getCart.receive]: (state, { payload, error }) => {
        if (error) {
            return {
                ...initialState,
                getCartError: payload
            };
        }

        return {
            ...state,
            cartId: String(payload)
        };
    },
    ...
}

// cart/asyncActions.js
export const getCartDetails = payload => {
    return async function thunk(dispatch, getState) {
        const { cart, user } = getState();
        const { getCartError } = cart;

        if (getCartError) {
            // unrecoverable!
            return;
        }
        ...
    }
}

[jimbo]

No, this looks fine. I think it's standard practice to notify the reducer of an error, so it can be added to state, then follow up by throwing the error (or rejecting with it, if we were using promises directly).

[sirugh]

Ok - for now I've added the cart error to the state slice even though we won't do anything outright with it.

[supernova-at]

It does seem preferable for these actions to check for and create a cart before attempting their own work instead of issuing a network request we know is going to fail and then doing the create + retry in a catch.

We could save ourselves processing time and network requests when the cart is bad.

And I don't think what @jimbo is saying contradicts that. This throw in createCart should stay, and also all the other actions could check for the presence of getCartError in state before attempting their work.

[devops-pwa-codebuild]

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2574.pwa-venia.com : LH Performance Expected 0.85 Actual 0.34, LH Accessibility Expected 1 Actual 0.97, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 39.333333333333
https://pr-2574.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.33, LH Best Practices Expected 1 Actual 0.92
https://pr-2574.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.39, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 65 Actual 50

[jimbo]

No, this looks fine. I think it's standard practice to notify the reducer of an error, so it can be added to state, then follow up by throwing the error (or rejecting with it, if we were using promises directly).

[supernova-at]

Consider adding a retry to createCart since it's so important that we have one. That said, I'm unsure how often immediately retrying will result in a different outcome.

Also consider having the cart actions check for getCartError and call createCart before attempting work instead of waiting for the action to fail.

All in all though, I consider those both "nice to haves". This is 👍

[supernova-at]

Is there a minimum number of retries we could try here?

Looks like some other actions make that decision based on error.networkError - we could retry a few times if it's a network error or something?

[sirugh]

I want to save retry logic for the apollo link rather than adding it to the actions that we know we are eventually going to remove. But more pertinently, as far as I know there are no possible errors from this mutation aside from server or network errors, which indicate a deeper issue. If you have a working network connection and the server is functioning, calling createCart will result in a new cart id or the users cart id if provided a bearer token. My point is that network/server errors should be handled by apollo-link-retry and not by redux actions.

[supernova-at]

It does seem preferable for these actions to check for and create a cart before attempting their own work instead of issuing a network request we know is going to fail and then doing the create + retry in a catch.

We could save ourselves processing time and network requests when the cart is bad.

And I don't think what @jimbo is saying contradicts that. This throw in createCart should stay, and also all the other actions could check for the presence of getCartError in state before attempting their work.