refactor callbacks #334
refactor callbacks #334
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| interface Subscription<T extends ModelTypes> { | ||
| event: T | ||
| callback: ( | ||
| data: Extract<Models['OkModelingCmdResponse_type'], { type: T }> | ||
| ) => void | ||
| } |
There was a problem hiding this comment.
I'm pretty happy with this typing, making the event narrow down the data arg in the callback, however . . .
| subscriptions: { | ||
| [event: string]: { | ||
| [localUnsubscribeId: string]: (a: any) => void | ||
| } | ||
| } = {} as any |
There was a problem hiding this comment.
however ... I was unable to get good types for this nested object of callbacks.
I had something like
subscriptions: {
[event in ModelTypes]?: {
[localUnsubscribeId: string]: Subscription<ModelTypes>['callback']
}
} = {}In mind, but that just caused me more headaches. I'm okay with this because it makes the authoring experience of actually adding a new subscription quiet good, (good types in the callback function)
But definitely could get better.
| const unSubClick = engineCommandManager.subscribeTo({ | ||
| event: 'select_with_point', | ||
| callback: ({ data }) => { | ||
| if (!data?.entity_id) { | ||
| setCursor2() | ||
| return | ||
| } | ||
| const sourceRange = sourceRangeMap[data.entity_id] | ||
| setCursor2({ range: sourceRange, type: 'default' }) | ||
| }, |
| const sourceRange = sourceRangeMap[id] | ||
| setHighlightRange(sourceRange) | ||
| } | ||
| const unSubHover = engineCommandManager.subscribeToLossy({ |
There was a problem hiding this comment.
Food for thought, not feedback on this line -- we call this construct (the UDP data channel in the WebRTC stream) a few things, like unreliable or lossy. I have no strong feelings, but it may be worth getting everything to use the same word everywhere to make things a bit more grepable; I think "Lossy" is the one that's won out on the codebase
| @@ -331,6 +331,23 @@ export class EngineConnection { | |||
| } | |||
|
|
|||
| export type EngineCommand = Models['WebSocketRequest_type'] | |||
| type ModelTypes = Models['OkModelingCmdResponse_type']['type'] | |||
|
|
|||
| type LossyResponses = Extract< | |||
There was a problem hiding this comment.
Extract is nice, this is cool
| result?.data.sequence > this.inSequence && | ||
| result.type === 'highlight_set_entity' | ||
| ) { | ||
| this.inSequence = result.data.sequence |
There was a problem hiding this comment.
Something I just thought of when working through this review (but not material to this PR since this exists in main), when we do a WebSocket/WebRTC reconnect, we may get a different engine-api, which means the sequence would be reset. I reckon we need to zero out inSequence when we have a EngineConnection open event
There was a problem hiding this comment.
I filed #336 for us to not forget this (although I haven't got reconnect implemented yet -- the new EngineConnection retry stuff is only on connect, not if a WebSocket breaks after connection, so we've never hit this yet, I don't think)
|
I landed this since I wound up reaching for something here and saw it wasn't there, it was approved and looks good locally |
I had two motivations for this.