Allowing the transports to transform StartBotResult into TransportConnectionParams when using startBotAndConnect. #29
Conversation
filipi87
changed the title
| let transportParams: T = await try self.startBot(startBotParams: startBotParams) | ||
| public func startBotAndConnect<T: StartBotResult>(startBotParams: APIRequest) async throws -> T { | ||
| let startBotResult: T = await try self.startBot(startBotParams: startBotParams) | ||
| let transportParams = try self.transport.transformStartBotResultToConnectionParams(startBotParams: startBotParams, startBotResult: startBotResult) |
There was a problem hiding this comment.
@mattieruth , this is basically the _validateConnectionParams we had on the web.
There, we didn’t pass startBotParams to avoid a breaking change, but since this is a completely new method here, I thought this approach would make more sense.
There was a problem hiding this comment.
What do you think ?
There was a problem hiding this comment.
hmm. actually, i think there's a bigger reason we don't do it this way on web. The issue is that we want the validation to occur as part of connect() and not part of startBot(). The above prevents someone from doing their own (i'm going to write this in js because... swift is alien): const result = await startBot(); waitForSomethingOrDoSomethingBeforeConnect(); connect(result); and still benefit from the transformation and validation.
There was a problem hiding this comment.
I thought the reason we ended up adding _validateConnectionParams inside connect on the web was due to all the backward compatibility we wanted to maintain there.
Here, it kind of feels like we have strong typing for both the startBot result and the connect parameters, which could actually be completely independent of each other.
That’s why I was only invoking this method to perform the transformation inside startBotAndConnect.
But let me know if you think having it inside connect is really the right approach here as well. If that’s the case, I believe I wouldn’t need a StartBotResult protocol, since startBot should also return TransportConnectionParams in this case.
Then, I will move to invoke the transformStartBotResultToConnectionParams method inside connect, and we should probably rename it to something else, maybe normalizeConnectionParams ? (Totally open to name suggestions; this one isn’t great. 😅)
What do you think?
Also tagging @marcus-daily, who might be interested in this for Android.
There was a problem hiding this comment.
I thought the reason we ended up adding _validateConnectionParams inside connect on the web was due to all the backward compatibility we wanted to maintain there.
Somewhat? But not completely. Especially now with these SmallWebRTC startBot returns. Now this is supporting existing and recommended approaches. Let's say I want to build my own start endpoint because I need to return some other parameters for my client to handle in addition to the ones our built-in endpoint in the runner provides. Then I would copy exactly what the runner does, add the parameters i need and have my client do that startBot() and connect() separately. But we also don't want to force people's startBot() to return a certain type. They should be able to do their own transformation from whatever they send back in order to generate transport params.
Maybe I'm being too flexible? Yell at me if I am or if I'm missing the point.
There was a problem hiding this comment.
But we also don't want to force people's startBot() to return a certain type. They should be able to do their own transformation from whatever they send back in order to generate transport params.
But we are allowing that. They can receive whatever they want from the startBot. That is why we are returning the StartBotResult protocol (which can be anything).
They can then transform it into SmallWebRTCTransportConnectionParams (in the SmallWebRTC case) and then invoke connect.
I just don't think that we need make the connect method, receive anything other than ConnectionParams, so SmallWebRTCTransportConnectionParams for the SmallWebRTC.
There was a problem hiding this comment.
but my transform function will have to re-create all the logic in transformStartBot... if I've copied our pipecat endpoint and just added a field, right?
There was a problem hiding this comment.
i guess it could just call transformStartBot... itself. It just might not be obvious to do that. 🤔
alright. i'm on board. i'm not sure i'll follow suit on web, though, since the types are so much looser there.
filipi87
changed the title
…nectionParams when using startBotAndConnect.
filipi87
force-pushed
the
release_1.1.1
branch
from
3f91463 to
d0912ac
Compare
Allowing the transports to transform StartBotResult into TransportConnectionParams when using startBotAndConnect.