-
Notifications
You must be signed in to change notification settings - Fork 683
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
fix: no more infinite error loop if cart creation fails #2574
Conversation
…ting the cart Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
8634860
to
df0100f
Compare
|
dispatch(actions.getCart.receive(error)); | ||
throw new Error('Unable to create cart'); |
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 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;
}
...
}
}
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.
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).
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.
Ok - for now I've added the cart error to the state slice even though we won't do anything outright with it.
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.
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.
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 |
dispatch(actions.getCart.receive(error)); | ||
throw new Error('Unable to create cart'); |
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.
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).
1363e0c
to
878ddc4
Compare
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
878ddc4
to
3cf83cd
Compare
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.
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 👍
dispatch(actions.getCart.receive(error)); | ||
throw new Error('Unable to create cart'); |
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.
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?
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 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.
dispatch(actions.getCart.receive(error)); | ||
throw new Error('Unable to create cart'); |
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.
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.
Signed-off-by: sirugh <rugh@adobe.com>
Description
Cart creation is currently handled by two actions.
getCartDetails
which callscreateCart
if there is no cart id when the app initializes. Unfortunately, if there is an error during thecreateCart
action, we only dispatched the error (which reset the cart slice to initial state wherecartId === null
) and then retried thegetCartDetails
action. This results in an infinite loop.The fix is to allow
createCart
to throw, but to gracefully handle the error and just return fromgetCartDetails
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
Screenshots / Screen Captures (if appropriate)
Checklist