-
Notifications
You must be signed in to change notification settings - Fork 15
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
Add info event to propagate protocol-level events #43
Conversation
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.
Thanks for this speedy enhancement!
@@ -19,11 +19,18 @@ type ConnectEvent = Partial<{ | |||
reconnect: boolean | |||
}> | |||
|
|||
export type InfoEvent = { | |||
type: string |
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.
Is it safe to assume this can actually just be:
type: string | |
type: 'info' |
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's not 'info', but a protocol-level event ('history_received' or 'history_not_found').
The idea is that we allow the protocol to propagate anything up to the cable and channels, so the possible event types are not known beforehand.
We can probably add some generics and TS magic to make the InfoEvent.typed inferred from the Cable.protocol, but... let's leave it as a potential contribution for some TS enthusiast :)
@@ -32,11 +32,17 @@ type ConnectEvent = Partial<{ | |||
reconnect: boolean | |||
}> | |||
|
|||
export type InfoEvent = { | |||
type: string |
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.
And here, could we possibly use a string union type to be more specific?
type: string | |
type: 'history_received' | 'history_not_found' |
Context
Our extended protocol supports automatic stream history retrieval but we do not provide any APIs to react on the corresponding events (more precisely, protocol messages:
confirm_history
andreject_history
).This PR introduces a new
info
event for cables and channels that can be used to signal arbitrary protocol-level events to users, includinghistory_not_found
andhistory_received
events: