-
Notifications
You must be signed in to change notification settings - Fork 154
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
Wishlist Update schemas with UID changes #454
base: master
Are you sure you want to change the base?
Wishlist Update schemas with UID changes #454
Conversation
It's a breaking change to remove arguments from any released schemas. Please see the guidance in this document https://github.com/magento/architecture/blob/master/design-documents/graph-ql/id-improvement-plan.md#fields-with-1-or-more-foreign-identifier-argument |
addProductsToWishlist(wishlistId: ID!, wishlistItems: [WishlistItemInput!]!): AddProductsToWishlistOutput | ||
removeProductsFromWishlist(wishlistId: ID!, wishlistItemsIds: [ID!]!): RemoveProductsFromWishlistOutput | ||
updateProductsInWishlist(wishlistId: ID!, wishlistItems: [WishlistItemUpdateInput!]!): UpdateProductsInWishlistOutput | ||
} | ||
|
||
type Customer { | ||
wishlist: Wishlist! @deprecated(reason: "Use `Customer.wishlists` or `Customer.wishlist_v2") | ||
wishlist_v2(id: ID!): Wishlist # This query will be added in the ce | ||
wishlist_v2(uid: ID!): Wishlist # This query will be added in the ce |
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.
We should just deprecate wishlist field and use new field wishlists, which will always have one item in Open Source.
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.
@melnikovi I see, you have updated the schema file, Is there a need to do any additional changes to the schema file?
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.
what he means here is we should either add wishlist_v3
However we decided to make id not required and add uid also as not required
wishlist_v2(id: ID, uid: ID) - in the code we will make at least 1 that's required but not through schema
adding wishlist_v3 for this is kind of intrusive.
@melnikovi @paliarush I have updated the PR with fixed Resolve Conflicts. |
addProductsToWishlist(wishlistId: ID!, wishlistItems: [WishlistItemInput!]!): AddProductsToWishlistOutput | ||
removeProductsFromWishlist(wishlistId: ID!, wishlistItemsIds: [ID!]!): RemoveProductsFromWishlistOutput | ||
updateProductsInWishlist(wishlistId: ID!, wishlistItems: [WishlistItemUpdateInput!]!): UpdateProductsInWishlistOutput | ||
} | ||
|
||
type Customer { | ||
wishlist: Wishlist! @deprecated(reason: "Use `Customer.wishlists` or `Customer.wishlist_v2") | ||
wishlist_v2(id: ID!): Wishlist # This query will be added in the ce | ||
wishlist_v2(uid: ID!): Wishlist # This query will be added in the ce |
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.
what he means here is we should either add wishlist_v3
However we decided to make id not required and add uid also as not required
wishlist_v2(id: ID, uid: ID) - in the code we will make at least 1 that's required but not through schema
adding wishlist_v3 for this is kind of intrusive.
@@ -33,8 +56,9 @@ type WishlistItems { | |||
} | |||
|
|||
input WishlistItemUpdateInput { | |||
wishlist_item_id: ID |
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.
we can't remove any of the existing fields, but deprecate them and also add wishlist_item_uid
@@ -1,6 +1,6 @@ | |||
type Customer { | |||
gift_registry_list: [GiftRegistry] | |||
gift_registry(id: ID!): GiftRegistry |
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.
we already took care of gift registy. please remove this scope from your PR
Schema has a lot of fields named id which doesn't align with the new naming convention of GraphQL
Updating all id fields to
uid
to be consistent with the new naming convention.Getting Reference from the PR, #435