MAGENTO-PWA#2190: Fixed dependent GraphQL schema according to best practices #27620
Conversation
|
Hi @andrewbess. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
ghost
added
the
magento-engcom-team
added
Component: QuoteGraphQl
Release Line: 2.4
Partner: Atwix
| @@ -20,7 +20,7 @@ type Mutation { | |||
| setPaymentMethodOnCart(input: SetPaymentMethodOnCartInput): SetPaymentMethodOnCartOutput @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\SetPaymentMethodOnCart") | |||
| setGuestEmailOnCart(input: SetGuestEmailOnCartInput): SetGuestEmailOnCartOutput @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\SetGuestEmailOnCart") | |||
| setPaymentMethodAndPlaceOrder(input: SetPaymentMethodAndPlaceOrderInput): PlaceOrderOutput @deprecated(reason: "Should use setPaymentMethodOnCart and placeOrder mutations in single request.") @resolver(class: "\\Magento\\QuoteGraphQl\\Model\\Resolver\\SetPaymentAndPlaceOrder") | |||
| mergeCarts(source_cart_id: String!, destination_cart_id: String!): Cart! @doc(description:"Merges the source cart into the destination cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\MergeCarts") | |||
| mergeCarts(input: MergeCartsInput): Cart! @doc(description:"Merges the source cart into the destination cart") @resolver(class: "Magento\\QuoteGraphQl\\Model\\Resolver\\MergeCarts") | |||
There was a problem hiding this comment.
I'm not sure about the backward compatibility of this change.
@lenaorobei, could you confirm, please, that we may perform such changes?
Thank you
There was a problem hiding this comment.
The implementation of mergeCarts was added based on approved proposal from the architecture repo https://github.com/magento/architecture/blob/master/design-documents/graph-ql/coverage/shared-cart.md
Need to understand how this will help PWA devs.
Addind @cpartica @paliarush
There was a problem hiding this comment.
@andrewbess could you please explain the isse with using mergeCarts(source_cart_id: String!, destination_cart_id: String!)?
There was a problem hiding this comment.
I believe the main concern here is extensibility. If we use a separate type for input (as we do in most cases), we may extend this input (add fields, for example) in a separate module. If we have the parameters hardcoded as they are now, the extensibility is compromised.
But the main question here is the backward compatibility. If some merchant already uses the existing mergeCarts mutation somehow, he will be surprised after the upgrade.
There was a problem hiding this comment.
Hello @rogyar
The GraphQL for merging carts looks like incorrect. It doesn't use Magento best practices.
I don't understand why it has been changed from the correct code to incorrect.
Please take a look at commits below.
Correct commit (IMHO)
magento/graphql-ce#950
Incorrect commit that changes previous (IMHO)
f19f727
I also want to note that this is a relatively new feature.
It appeared only in version 2.3.4
There was a problem hiding this comment.
Hello @lenaorobei
Thank you for your question.
This mutation isn't written according to Magento best practices.
Currently, I am working on PWA-studio#2190.
So, I think it would be better to change it now and don't alter PWA after changing this mutation in the future.
There was a problem hiding this comment.
@rogyar regarding your concern about extensibility - both options should work - adding new field and parameter.
@andrewbess there was an issue on the DevDocs page that is already addressed magento/devdocs#7001
This recommendation was outdated.
Please refer to the extensibility approach in the architecture repo https://github.com/magento/architecture/blob/master/design-documents/graph-ql/extensibility.md
There was a problem hiding this comment.
Hi @lenaorobei. Thank you for clarification. We used to use containers for input arguments for merging purposes. So containers with the same name from different modules were supposed to be merged. If you can confirm that the raw input parameters approach (with no containers) has merging capabilities, then we good to go.
Thank you!
There was a problem hiding this comment.
@rogyar yes, mutations/queries with the same name are supposed to be merged. This is the way of adding new parameters.
I would really appreciate if someone can double check that.
|
Closing this PR. |
|
Hi @andrewbess, thank you for your contribution! |
Description (*)
This PR fixes GraphQL schema for merging carts according to Magento best practices
Related Pull Requests
Fixed Issues (if relevant)
Questions or comments
Contribution checklist (*)