Add chat API support #278
Add chat API support #278
Conversation
🦋 Changeset detectedLatest commit: 3183086 The changes in this PR will be included in the next version bump. This PR includes changesets to release 6 packages
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 |
| @@ -185,6 +190,60 @@ export class LocalParticipant extends Participant { | |||
| }); | |||
| } | |||
|
|
|||
| async sendChatMessage( | |||
| text: string, | |||
| destinationIdentities?: Array<string>, | |||
There was a problem hiding this comment.
does this default to all? Worth adding docstrings
| async editChatMessage( | ||
| editText: string, | ||
| originalMessage: ChatMessage, | ||
| destinationIdentities?: Array<string>, |
There was a problem hiding this comment.
seems kinda weird that you can give different values here than to the original. this might also cause participants who join a call after an initial message is sent but before an edit to receive the edit and maybe have strange behavior? not sure if there's an easy way to solve though
There was a problem hiding this comment.
yeah, had the same feeling and opted to omit destinationIdentites on client-sdk-js altogether for now because of that
| export interface ChatMessage { | ||
| id: string; | ||
| timestamp: number; | ||
| message: string; | ||
| editTimestamp?: number; | ||
| generated?: boolean; | ||
| } |
There was a problem hiding this comment.
as far as i understand this is the same as the proto interface but with bigints swapped out for numbers? there's already a bunch of conversions happening in code and i wonder if this cuts down on enough of them to warrant this utility type or whether it's okay to just use the proto one
There was a problem hiding this comment.
are you concerned about performance reasons of the conversions or maintenance burden?
reasons for me to keep them separate:
- we might be adding stuff in the proto that we don't necessarily want to expose to the user facing API in the same form
- it's a bit of a pain to work with bigints in JS and we're just shifting the conversion burden to userland if we don't do the conversion ourselves
There was a problem hiding this comment.
mix of maintenance burden + consistency with other things, this is i think the first time we do this conversion to a special type, and it's a weak point if the proto ever changes — this adds a second source of truth that isn't dependent on the other and is mostly redundant
i agree that doing the bigint juggling ourselves is the right choice, though. just a bit weird how this is the first time we've had to do this
There was a problem hiding this comment.
just a bit weird how this is the first time we've had to do this
FWIW in the client js sdk we do this for almost every proto type.
One downside of using the proto interfaces is that it is a complete protobuf class with conversion methods to/from JSON
|
also add a changeset |
depends on livekit/rust-sdks#436