-
Notifications
You must be signed in to change notification settings - Fork 114
Move RPC handlers to room #599
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
Conversation
|
it seems like you haven't added any nanpa changeset files to this PR. if this pull request includes changes to code, make sure to add a changeset, by writing a file to refer to the manpage for more information. |
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.
lgtm please add a changeset and add deprecated methods if not too much work
| } | ||
| } | ||
|
|
||
| pub fn register_rpc_method( |
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.
can we keep deprecated stubs of these around that just forward to the new room method? that way its not a breaking change
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.
@bcherry
Oh, I was thinking of covering this on the foreign language side.
I didn’t want to support the old interface from rust side, since that would mean we’d have to keep the old proto definitions around and start adding things like RegisterRpcMethodRequest2, which I’d rather avoid. What do you think?
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 don't think you need to expose the old interface over FFI (agree you can just handle it in python/node), but the underlying rust rtc sdk would ideally still offer it. i don't think there'd be extra proto definitions around in that case
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 see — you mean keeping the old interface on the livekit crate, not in livekit-ffi. That makes sense to me. Will do!
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.
Added in cf40cb9
No description provided.