Skip to content
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 type guards for ws events #190

Open
tiagosiebler opened this issue Dec 26, 2021 · 5 comments
Open

Add type guards for ws events #190

tiagosiebler opened this issue Dec 26, 2021 · 5 comments

Comments

@tiagosiebler
Copy link
Owner

Implementation would be easier for websockets if the library included pre-built type guards to narrow down types.

export function isWsFuturesUserDataEvent(
  data: WsFormattedMessage
): data is WsMessageFuturesUserDataEventFormatted {
  // The wsKey can be parsed to determine the type of message (what websocket it came from)
  return !Array.isArray(data) && data.wsKey.includes('userData');
}

Could put these under util/typeGuards.ts

tiagosiebler pushed a commit that referenced this issue Dec 27, 2021
tiagosiebler pushed a commit that referenced this issue Dec 28, 2021
tiagosiebler added a commit that referenced this issue Dec 28, 2021
add type guards for ws events (#190), add checks for duplicate user data streams & timers (fixes #191)
@jule64
Copy link
Contributor

jule64 commented Dec 8, 2022

Hi Tiago,

I was looking at the type guards and created two new ones that I needed for my implementation. I have added the names and signature below and can send a PR if they look ok (I can also try to add more but wanted to check that those two are correct before I add more).

function isWsAggTradeFormatted(data: WsFormattedMessage): data is WsMessageAggTradeFormatted
function isWsSpotUserDataExecutionReportFormatted(data: WsUserDataEvents):
 data is WsMessageSpotUserDataExecutionReportEventFormatted

I also wanted to check something that confused me a little bit with the messages types: basically while implementing the type guards below I noticed that I could pass a WsUserDataEvents to a function that expects a WsFormattedMessage.

This seems odd to me as I thought the two message types are supposed to be disctinct/exclusive. But maybe my understanding is incorrect.
I do see that one of the sub types of WsUserDataEvents is also a subtype of WsFormattedMessage so wondering if that's the reason for this "behaviour" (the subtype is WsMessageFuturesUserDataEventFormatted).

@jule64
Copy link
Contributor

jule64 commented Dec 8, 2022

Ok I think I found the answer to my question regarding the WsUserDataEvents being a subtype of WsFormattedMessage. I think that is expected since any 'formattedUserDataMessage' event is also a 'formattedMessage' in src/websocket-client.ts:410

@tiagosiebler
Copy link
Owner Author

It's possible a mistake on these types may have slipped through. Technically user data events can also be raw and/or formatted, like any other event from binance. If WsUserDataEvents is the union type of all formatted user data events, I would have preferred if I labelled it WsFormattedUserDataEvents (unless I realised it later and didn't want a breaking change yet...)

Looking at types/websockets.ts, I see it's a union of all formatted spot user data events and all formatted futures user data events. So yes, definitely regret the choice in naming here - I would be tempted to duplicate it with a better named version and mark the current one as deprecated for removal in the next major release.

Generally I've wanted it to be obvious which interface or type represents a formatted vs raw message, so there's no confusion or digging around during implementation.

Regarding your question on the type guards, yes, I'd absolutely love to see more being added.

Since the 'formattedMessage' emitter always emits WsFormattedMessage, this is the data type of the input parameter for any type guard on formatted events:
https://github.com/tiagosiebler/binance/blob/master/src/websocket-client.ts#L94-L97

This can be seen in the type guards so far: https://github.com/tiagosiebler/binance/blob/master/src/util/typeGuards.ts

Hope that clears out the questions, but let me know if there's more! Ideas are also welcome of course.

@jule64
Copy link
Contributor

jule64 commented Dec 8, 2022

Ok I think that makes sense. So regarding the second typeguard isWsSpotUserDataExecutionReportFormatted do I need to change the input from data: WsUserDataEvents to data: WsFormattedMessage ? And is this the expected pattern for the typeguards in this file?

And if that's the case then I wonder if there might be a way to constrain this type at file level (or have a class with a generic type T of WsFormattedMessage with relevant typeguards in it)? Sorry thinking out loud here and not really used to typescript patterns yet

@tiagosiebler
Copy link
Owner Author

Ok I think that makes sense. So regarding the second typeguard isWsSpotUserDataExecutionReportFormatted do I need to change the input from data: WsUserDataEvents to data: WsFormattedMessage ? And is this the expected pattern for the typeguards in this file?

And if that's the case then I wonder if there might be a way to constrain this type at file level (or have a class with a generic type T of WsFormattedMessage with relevant typeguards in it)? Sorry thinking out loud here and not really used to typescript patterns yet

I think the inputs to all these type guards should be the highest level union type (or unknown if there's any doubt on the type), since that allows these type guards to be used at the highest level of the event handler (so you don't need to call other type guards first if you don't want to).

Repetition can be reduced by combining type guards, for example this one works on the highest level but also does some of the repetitive checks via another guard:

export function isWsFormattedSpotUserDataEvent(
  data: WsFormattedMessage
): data is WsMessageSpotUserDataEventFormatted {
  return isWsFormattedUserDataEvent(data) && data.wsMarket.includes('spot');
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants