-
Notifications
You must be signed in to change notification settings - Fork 8
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
feat(Simulations): narrow down simulation payload types #52
Conversation
type TransactionUpdatedEvent, | ||
} from '../../notifications/events'; | ||
|
||
export interface EventPayloadMap { |
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 export this? If so, would SimulationUpdatePayloadMap
be clearer?
EventPayloadMap
feels quite generic
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.
Nope, doesn't need to be exported, how about SimulationEventPayloadMap
?
'report.updated': ReportUpdatedEvent; | ||
} | ||
|
||
export type DiscriminatedEventResponse<Base> = { |
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.
Similar naming here, should we include Simulation
somewhere?
notificationSettingId?: string; | ||
name?: string; | ||
status?: Status; | ||
type?: IEventName | SimulationScenarioType; |
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.
Another type
here we should reenable.
id: string; | ||
status: Status; | ||
notification_setting_id: string; | ||
name: string; | ||
type: IEventName | SimulationScenarioType; |
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.
Maybe another type
here?
* feat(Simulations): narrow down simulation payload types * refactor(Simulations): rename event map * fix(Simulations): Include scenarios in union * refactor(Simulations): rename DiscriminatedEventResponse
* feat(SimulationTypes): Add simulation types resource * chore: remove payment_method events * feat(Simulations): Add simulation resource * chore(Simulations): update request body types * chore: remove unused import * feat(Simulations): Add simulation runs resources * feat(Simulations): Add simulation run events resources * chore: clean up unused vars * feat(Simulations): narrow down simulation payload types (#52) * feat(Simulations): narrow down simulation payload types * refactor(Simulations): rename event map * fix(Simulations): Include scenarios in union * refactor(Simulations): rename DiscriminatedEventResponse * chore: bump version * fix(Simulations): update imports * chore: update import
Based on some recommendations after creating #51 it is a much more common pattern in this lib to use discriminated unions for this use case, originally this would have look like this:
naturally this ended up being quite exhaustive when required for
ISimulationResponse
,CreateSimulationRequestBody
andUpdateSimulationRequestBody
so this looks at an intermediary by using a helper to generate the discriminated union based on a base interfaceThe main downside to using discriminated unions over generics as looked at in #51 is that there is no way to quickly type the get simulation request other than coercing it