-
Notifications
You must be signed in to change notification settings - Fork 38
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
Support storefront api for customer account api #1465
Support storefront api for customer account api #1465
Conversation
0ec09fc
to
520d61a
Compare
We detected some changes in |
*/ | ||
query: <Data = unknown, Variables = {[key: string]: unknown}>( | ||
query: string, | ||
options?: {variables?: Variables; version?: StorefrontApiVersion}, |
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.
Have we decided on not supporting query for customers API with an optional parameter ?
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 are only going to support storefront api for now, for query on customer API, we need to figure out how to do that for soft auth ( which customer should the query represents, how to add soft auth authentication header ), and whether to update existing query api ( currently we have to match order status api ), if we want to, maybe another parameter, or another function
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.
Looks good 👍🏼 Should we make the OrderStatus targets extend the standard api interface on unstable ? It would avoid having to duplicate the query
typings here (although it will be a much bigger lift)
I have to keep query in OrderStatusApi for now, and we are not going to extend standardApi in orderstatus api, instead , we will have all order status related targets have both orderStatusApi and standardApi, and then remove duplicated one ( not not intended to override ones ), I will have a PR open soon |
520d61a
to
c3aaf8a
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.
🎩 LGTM
Background
Part of Shopify/core-issues#61073
Please also have a review of customer account web side PR https://github.com/Shopify/customer-account-web/pull/3246
Support storefront api for standard customer account ui extension api
Solution
(Describe your solution, why this approach was chosen, and what the alternatives/impacts may be)
🎩
Please see the tophat here https://github.com/Shopify/customer-account-web/pull/3246
Checklist