Proposal: Deprecate `Mutation.createEmptyCart`, add `Mutation.createGuestCart` #385

DrewML · 2020-06-16T19:44:15Z

sirugh · 2020-06-16T20:03:38Z

design-documents/graph-ql/coverage/deprecate-createEmptyCart-add-createCart.md

+type Mutation {
+    createCart(
+        input: CreateCartInput
+    ): CreateCartOutput @doc(description: "Create a new shopping cart")


The current createEmptyCart mutation has some magic to it. If the request is authenticated with a bearer token the mutation returns the user's existing cart id otherwise it returns a new cart's id.

I see in the new response type the cart is returned, which is ideal, but would it also still return a user's existing cart?

If the request is authenticated with a bearer token the mutation returns the user's existing cart id otherwise it returns a new cart's id.

Does it return the existing cart ID even in the case where that cart is not empty? If so that makes the createEmptyCart name a bit counter-intuitive.

Does it return the existing cart ID even in the case where that cart is not empty

Checked, it does.

createCartForCustomer->execute in resolver

createCustomerCart in createEmptyCartForCustomer

quoteRepository->getActiveForCustomer in createCustomerCart

That's confusing! I think the name is more confusing than the API, because you wouldn't want to flush a customer's existing cart, but a customer can only have a single cart.

I wonder what a better name is here? A name that makes it clear that this returns an existing thing, or falls back to creating a new thing.

Does it return the existing cart ID even in the case where that cart is not empty?

Yes, it returns the user's existing cart id in both cases.

If so that makes the createEmptyCart name a bit counter-intuitive.

And yes, I agree. That's why we alias over it in our code.

I wonder what a better name is here? A name that makes it clear that this returns an existing thing, or falls back to creating a new thing.

In PWA Studio a user can only have one cart visible and active. We have no concept of fetching data for a "stashed" cart. So I can't see why you ever need the concept of new cart. I would think that simply querying for cart would suffice regardless. You're either getting a new cart or an active one if authed.

Sorry - shouldn't have even mentioned "stashed cart". I don't even know if Magento has the idea of a customer having multiple carts at once.

getCart seems reasonable, but the name sounds more like a query.

Went and looked at venia a bit more, and now I'm questioning if createEmptyCart handling logged-in users makes sense at all.

@sirugh what's the situation where we would want to use createEmptyCart for a logged-in user? It seems like, at the time venia sends this mutation, we already should know if the user is a guest or not, right?

I think these are the cases we need to cover, but please correct me if I'm wrong:

When rendering the cart

Use Case Covered by

User logged-in? Query.customerCart

User not logged in, but guest cart token in local storage? Query.cart(id: ID)

User not logged in, and no guest cart token in local storage? Mutation.createEmptyCart, which would return the full Cart type

If the above is correct, is seems like createEmptyCart should be something like createGuestCart. Thoughts?

getCart seems reasonable, but the name sounds more like a query.

Yeah, I keep wanting to say it, but then stop myself each time because it doesn't make it clear that it causes side-effects.

Hopefully my proposal above this comment improves things?

@sirugh what's the situation where we would want to use createEmptyCart for a logged-in user? It seems like, at the time venia sends this mutation, we already should know if the user is a guest or not, right?

I don't remember why we used the single mutation but it seemed to make the logic simpler.

If the above is correct, is seems like createEmptyCart should be something like createGuestCart. Thoughts?

Sure that sounds fine.

I don't remember why we used the single mutation but it seemed to make the logic simpler.

Makes sense. I guess we ended up just moving the logic into an odd place: a createEmptyCart query that can return a non-empty cart.

I pushed up some changes to the proposal based on this thread. Can you give it another read through?

…tCart

nrkapoor

@DrewML Looks good to me. Can you confirm this is not a breaking change?

DrewML · 2020-06-17T14:58:41Z

@nrkapoor Can confirm - not a breaking change. Only change to an existing API is marking for deprecation, but not strict requirement to remove until down the line when we're sure usage has mostly dropped off.

nrkapoor · 2020-06-17T22:39:22Z

@nrkapoor Can confirm - not a breaking change. Only change to an existing API is marking for deprecation, but not strict requirement to remove until down the line when we're sure usage has mostly dropped off.

@DrewML Thanks. Please let me know when we are ready to merge this change so I can create a ticket on the graphql board for implementation.

paliarush

Until some point createEmptyCart was used by both, guests and registered customers. After creating separate query for registered customers, it was not renamed because of backward compatibility.

paliarush · 2020-07-27T22:11:28Z

design-documents/graph-ql/coverage/deprecate-createEmptyCart-add-createGuestCart.md

+1. Requires manual cache handling with libraries like Apollo, to link the `ID` back to a `Cart` object
+2. Requires 2 round trips for the client to get cart data for a guest. Even though the cart is empty, it still has defaults the pwa-studio components will use to render an empty cart
+
+During discussions, it was also noted that `createEmptyCart` has some behavior that's not clearly described by our schema. That is, If a customer is logged-in and has items in the cart, `createEmptyCart` returns an ID referencing a cart that is _not_ empty, which is extremely unintuitive. It was mentioned that the `pwa-studio` code [aliases this query](https://github.com/magento/pwa-studio/blob/38d652a4fbc797a4ac8ac0c3efa611003152c090/packages/venia-ui/lib/queries/createCart.graphql#L4) because of that.


This sounds like a bug. Registered customers are supposed to use customerCart query to get cart object (either new or existing)

design-documents/graph-ql/coverage/deprecate-createEmptyCart-add-createGuestCart.md

Co-authored-by: Alex Paliarush <[email protected]>

DrewML requested review from cpartica, nrkapoor, paliarush and sirugh June 16, 2020 19:44

DrewML force-pushed the deprecate-createEmptyCart-add-createCart branch from 49d8186 to 7363074 Compare June 16, 2020 19:46

sirugh reviewed Jun 16, 2020

View reviewed changes

DrewML force-pushed the deprecate-createEmptyCart-add-createCart branch 6 times, most recently from 299edb9 to c734a22 Compare June 16, 2020 22:00

DrewML changed the title ~~Proposal: Deprecate createEmptyCart, add createCart~~ Proposal: Deprecate Mutation.createEmptyCart, add Mutation.createGuestCart Jun 16, 2020

DrewML force-pushed the deprecate-createEmptyCart-add-createCart branch 4 times, most recently from ff9e700 to e75d77f Compare June 16, 2020 22:05

Proposal: Deprecate Mutation.createEmptyCart, add Mutation.createGues…

7d6e681

…tCart

DrewML force-pushed the deprecate-createEmptyCart-add-createCart branch from e75d77f to 7d6e681 Compare June 16, 2020 22:06

nrkapoor approved these changes Jun 17, 2020

View reviewed changes

DrewML added the open discussion Proposal or design document is open for public discussion label Jun 17, 2020

paliarush approved these changes Jul 27, 2020

View reviewed changes

paliarush added the GraphQL label Jul 28, 2020

melnikovi reviewed Sep 11, 2020

View reviewed changes

design-documents/graph-ql/coverage/deprecate-createEmptyCart-add-createGuestCart.md Outdated Show resolved Hide resolved

Apply suggestions from code review

e5481f5

Co-authored-by: Alex Paliarush <[email protected]>

melnikovi merged commit 62b50fc into master Sep 11, 2020

Use Case	Covered by
User logged-in?	`Query.customerCart`
User not logged in, but guest cart token in local storage?	`Query.cart(id: ID)`
User not logged in, and no guest cart token in local storage?	`Mutation.createEmptyCart`, which would return the full `Cart` type

Proposal: Deprecate Mutation.createEmptyCart, add Mutation.createGuestCart #385

Proposal: Deprecate Mutation.createEmptyCart, add Mutation.createGuestCart #385

Uh oh!

Conversation

DrewML commented Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirugh Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML Jun 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nrkapoor left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DrewML commented Jun 17, 2020

Uh oh!

nrkapoor commented Jun 17, 2020

Uh oh!

paliarush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Proposal: Deprecate `Mutation.createEmptyCart`, add `Mutation.createGuestCart` #385

Proposal: Deprecate `Mutation.createEmptyCart`, add `Mutation.createGuestCart` #385

DrewML commented Jun 16, 2020 •

edited

Loading

sirugh Jun 16, 2020 •

edited

Loading

DrewML Jun 16, 2020 •

edited

Loading

DrewML Jun 16, 2020 •

edited

Loading

nrkapoor left a comment •

edited

Loading