-
Notifications
You must be signed in to change notification settings - Fork 0
fix: enforce limit of 2 auction triggers at compile time #176
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
base: main
Are you sure you want to change the base?
Conversation
jbergstroem
left a comment
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.
Left feedback. Also.. tests.
|
@jbergstroem changed to compile time validation, we'll need to version bump though. |
| @@ -0,0 +1,59 @@ | |||
| /** | |||
| * Compile-time validation tests for auction constraints | |||
| * This file should compile without errors if the type system is working correctly | |||
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.
how do we then test invalid combinations?
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.
// @ts-expect-error can be an exit
I've changed the interface to accept the wrong combination and it fails (it expects an error, there's no error, so the annotation fails the "test")
| auctions: [{ type: "banners", slots: 1, slotId: "slot123", category: { id: "cat123" } }], | ||
| }; | ||
|
|
||
| const _invalid3TriggersAuction: Auction = { |
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.
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.
Wouldn't this allow any error, not just the error we're looking for? Are we sure this is the right path?
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.
added which specific error we're looking for 👍
|
Can we simplify your types? The permutations looks complicated. How about something like: type CategoryType =
| AuctionSingleCategory
| AuctionMultipleCategories
| AuctionDisjunctiveCategories;
type AuctionConstraints =
| { category: CategoryType; products: AuctionProduct; searchQuery?: never }
| { category: CategoryType; products?: never; searchQuery: string }
| { category?: never; products: AuctionProduct; searchQuery: string }
| { category: CategoryType; products?: never; searchQuery?: never }
| { category?: never; products: AuctionProduct; searchQuery?: never }
| { category?: never; products?: never; searchQuery: string }
| { category?: never; products?: never; searchQuery?: never };
type AuctionBase = AuctionBaseFields & AuctionConstraints;
interface SponsoredListingAuction extends Omit<AuctionBase, "type"> {
type: "listings";
}
interface BannerAuction extends Omit<AuctionBase, "type"> {
device?: DeviceType;
slotId: string;
type: "banners";
}
export interface Auction {
auctions: (SponsoredListingAuction | BannerAuction)[];
} |
The SDK allows an auction to have 3 triggers, but the API allows at most two.
Adde compile time validation to ensure correct formation.