-
Notifications
You must be signed in to change notification settings - Fork 165
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
Support SIP DTMF data messages #1130
Conversation
🦋 Changeset detectedLatest commit: f577316 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
size-limit report 📦
|
src/room/RTCEngine.ts
Outdated
} else { | ||
if (dp.value?.case === 'user') { | ||
// compatibility | ||
if (dp.participantIdentity === '' && dp.value.value.participantIdentity !== '') { |
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.
nice!
just to make it a bit easier to maintain, can we extract the compatibility stuff into a function?
it seems the pattern is always dp[myProperty] || dp.value.value[myProperty]
(empty strings are falsy in JS). Not sure if it's still elegant with typescript stuff, but having it extracted into a helper of some sort (not necessarily the one I proposed) would be nice!
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 looks like we might remove the compatibility part for the transcriptions (see livekit/protocol#706).
But in general, sure! I assume empty arrays are falsy too, right?
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.
But in general, sure! I assume empty arrays are falsy too, right?
no, empty arrays are truthy :D my naive proposal won't work for both strings and arrays...
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.
I added the helper functions you mentioned, but I get some TS error about DataPacket
type not supporting indexing. I might need your help on this one.
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.
I had a look and my initial idea might be totally flawed. I don't really see a way to get around the typescript issues without either being overly specific about the allowed strings or converting the protobuf to JSON first and then parsing it back. Which seems pointless and not at all simplifying things.
While verbose, your original implementation of handling these separately looks a lot preferable to me than back and forth parsing.
I think the main reason why I pushed for that was because we had logic duplication for transcription and user packet, but it seems that is no longer the case, because the field got renamed for transcriptions to transcribed_participant
. So I don't see a huge incentive anymore to change what you had originally.
I pushed to a branch what would work, but that's basically just extracting what you had previously already into a function 🤦 42b8381
Feel free to ignore it entirely.
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.
Yeah, that looks very specific. OK, lets settle on the middle ground - I'll revert the "generic" part of the change, but will keep compat logic in a separate function, so that it's easier to read the main handler function.
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.
Done, pushed the updated function. Also simplified logic a bit.
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.
nice!
Added support for SIP DTMF data messages on rooms and participants.
For consistency with other SDKs, the
RTCEngine
data packet handler signature was changed to receive all message types, which is then switched by type in the Room.Also compatibility code was added to properly select participant from a shared
participant_identity
on theDataPacket
(see livekit/protocol#706).