[PWA-335] Ensure apollo cache is updated after mutations/queries #2250
Conversation
- Re-factor PDP configurable mutation to use cart fragment
- Handle simple product and sign in flows with cart page fragment - Remove overfetching policies from CartPage
| fetchCartDetails | ||
| }); | ||
| }, [fetchCartDetails, fetchCartId, getCartDetails]); | ||
| if (!cartId) { |
There was a problem hiding this comment.
Previously this happened somewhat "magically", by calling the getCartDetails action, which then created a cart if one was not present. I just made it more explicit w/o mucking much with the redux portion of this logic.
There was a problem hiding this comment.
I tried this in a spike but I kept it at the root level like in useCartContext. It's one of those core assumptions of using the cart context - that you'll always have a valid cart, so I felt like this effect here should just come bundled with the context hook. We should avoid having cart creation be a part of each component that wants to use a cart or we will end up with multiple mutation requests for the id...which will be super slow now that we queue them all 😨
There was a problem hiding this comment.
Discussed some more on Slack, and I've decided to revert this change. We'll limit the scope of this PR and tackle larger architecture questions surrounding cart creation and retry logic in a future Story.
m2-community-project
bot
moved this from Ready for Review
to Review in Progress
in Pull Request Progress
| export const CartTriggerFragment = gql` | ||
| fragment CartTriggerFragment on Cart { | ||
| id | ||
| total_quantity |
There was a problem hiding this comment.
This ended up being interesting; total_quantity was already in CartPageFragment, so when I made this change, cart counter started updating w/o me including it explicitly alongside CartPageFragment. Is it okay to rely on that, or should we explicitly add CartTriggerFragment to every mutation as well?
There was a problem hiding this comment.
Is it possible that we were wrong about needing to re-use named mutations? Maybe it's hitting the cart cache because of the query signature. If that were the case fragment re-use would only be helpful in composed components where the parent requests all the children fragments. Cart Page, for example.
Edit: Ah wait I think I forgot the original intent - we need to update the trigger when something else modifies the underlying data.
This is a really good question :D
There was a problem hiding this comment.
Good question for sure.
We should ensure that mutations select any fields that may change as a result of that mutation in their response query; even if their coupled component does not need those fields for display.
If we do that I think it's perfectly fine to rely on the CartPageFragment's selection of total_quantity. The cart page is saying both: "I need this field for my display" and "some of my mutations update this field".
If some day in the future the cart page no longer needs total_quantity for its display, it should still include it if any of its mutations result in total_quantity changing (specifically, the mutation whose action results in total_quantity changing should be sure to include it).
If no such mutation exists and total_quantity is removed entirely from CartPageFragment, we can be confident that cartTrigger will always be up to date because we know that nothing else is updating that underlying data, as @sirugh pointed out above.
Long story short, we should not add CartTriggerFragment to every mutation.
There was a problem hiding this comment.
We should ensure that mutations select any fields that may change as a result of that mutation in their response query; even if their coupled component does not need those fields for display.
I'm not sure I agree, but I don't see a way around this.
We're essentially establishing a subscription model here. We can accomplish that with refetch queries or by including data in the mutation for things that "may change". I'm not sure I agree with the inclusion of response data in a mutation. It's a little too magical. That said, the alternative is using refetchQueries which is essentially the same thing.
"When something modifies this data, I need to know so I can update!"
In this case, we've got several components saying that they need to know about updates to some value. Which component is the "right" one? What if developers roll their own cart page instead of using ours? Maybe we should just always fetch all possible cart data after mutations?
|
tjwiebell
added
version: Minor
| export const CartTriggerFragment = gql` | ||
| fragment CartTriggerFragment on Cart { | ||
| id | ||
| total_quantity |
There was a problem hiding this comment.
Good question for sure.
We should ensure that mutations select any fields that may change as a result of that mutation in their response query; even if their coupled component does not need those fields for display.
If we do that I think it's perfectly fine to rely on the CartPageFragment's selection of total_quantity. The cart page is saying both: "I need this field for my display" and "some of my mutations update this field".
If some day in the future the cart page no longer needs total_quantity for its display, it should still include it if any of its mutations result in total_quantity changing (specifically, the mutation whose action results in total_quantity changing should be sure to include it).
If no such mutation exists and total_quantity is removed entirely from CartPageFragment, we can be confident that cartTrigger will always be up to date because we know that nothing else is updating that underlying data, as @sirugh pointed out above.
Long story short, we should not add CartTriggerFragment to every mutation.
m2-community-project
bot
moved this from Review in Progress
to Reviewer Approved
in Pull Request Progress
|
✅ Verification steps. Except:
I noticed a quick loading indicator. |
|
@tjwiebell Some strange issue happening only with account john.doe@example.com/123123^q, does not happen with some new account i created john.doe2033@example.com/123123^q. Steps -
|
|
Above issue because of gift card and we have separate issue to track that. |
m2-community-project
bot
moved this from Reviewer Approved
to Done
in Pull Request Progress
Description
Right now we have part of the app making mutations or queries to graphql without any knowledge of the other parts of the app. These should all be modified to use named fragments to prevent over fetch. For example, {{addItemToCart}}/{{removeItemFromCart}} could use a {{CartTriggerFragment}} and should definitely be using the {{CartPageFragment}} so that the cart trigger/page is always up to date. Right now the trigger has to manually refetch details and without using the cart page fragment the cart page won't be updated to reflect the proper state of the cart after the mutation.
So, where applicable, replace refetchQueries and incomplete query response bodies in product/cart mutations/queries with named fragments such as {{CartPageFragment}} or {{CartTriggerFragment}}.
Development Note
The lines between another very similar task PWA-321 were hard to align with, so most of the scope of that task has crept into this one a bit. It should work now that CartPage should be able to solely rely on whats in the cache, regardless of where the mutation occurred.
Related Issue
Acceptance
Verification Stakeholders
Specification
Verification Steps
/cartand verify all data is displayed, no loading indicator was renderedScreenshots / Screen Captures (if appropriate)
Checklist