-
Notifications
You must be signed in to change notification settings - Fork 51
Update API and add observables for discount function settings #3641
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
Update API and add observables for discount function settings #3641
Conversation
c5ef73a to
bf5f2f0
Compare
5747eaa to
6546c68
Compare
packages/ui-extensions/src/surfaces/admin/api/discount-function-settings/launch-options.ts
Show resolved
Hide resolved
f3ec9c3 to
6506b29
Compare
packages/ui-extensions/src/surfaces/admin/api/discount-function-settings/launch-options.ts
Outdated
Show resolved
Hide resolved
packages/ui-extensions/src/surfaces/admin/api/discount-function-settings/launch-options.ts
Outdated
Show resolved
Hide resolved
| Order = 'ORDER', | ||
| Shipping = 'SHIPPING', | ||
| } | ||
| type DiscountClass = 'product' | 'order' | 'shipping'; |
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 need to call this out in the changset as a breaking change. We will also need to ensure backwards compatibility so that only 2026-01 and up will receive the lowercased enum.
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.
Do we need to document this as a breaking change? This type was defined but never used. Like the input never referenced that type 🤔 It wasn't flagged as unused in CI because it is exported I assume.
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.
Ah ok, we've never exposed this in the api, just added the enum so it's ok to not be a breaking change
6ad4bcb to
ea8483b
Compare
ea8483b to
da8dfc7
Compare
| export interface SubscribableDiscountFields { | ||
| discountClasses: ReadonlySignalLike<DiscountClass[]>; | ||
| updateDiscountClasses: UpdateSignalFunction<DiscountClass[]>; | ||
| discountMethod: ReadonlySignalLike<DiscountMethod>; |
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.
discountMethod is readonly.
| * The object that exposes the validation with its settings. | ||
| */ | ||
| export interface DiscountFunctionSettingsData { | ||
| id: Discount; |
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.
This was actually wrong in the past. We never sent the id as an object with a single id property.
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.
Can we please patch the previous release too?
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.
It has been wrong for more than one release. I can patch all of them, do I just have to merge the branch to the non-rc one? Like 2025-10, etc. Anything else I need to do?
da8dfc7 to
f353ff9
Compare
|
@vividviolet ready for another review. Added a comment about the discountClasses type. |
| Order = 'ORDER', | ||
| Shipping = 'SHIPPING', | ||
| } | ||
| type DiscountClass = 'product' | 'order' | 'shipping'; |
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.
Ah ok, we've never exposed this in the api, just added the enum so it's ok to not be a breaking change
| * The object that exposes the validation with its settings. | ||
| */ | ||
| export interface DiscountFunctionSettingsData { | ||
| id: Discount; |
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.
Can we please patch the previous release too?
f353ff9 to
0d85224
Compare
0d85224 to
9440bde
Compare
Background
Related to https://github.com/Shopify/ui-api-design/pull/1332
Adds a subscribable API for Discounts Functions Settings extension target.