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

Abstract signaling server client #3

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

byrond
Copy link

@byrond byrond commented Nov 20, 2023

This is a draft of a PR that will ultimately be submitted upstream to yjs/y-webrtc

Refactors SignalingConn and related code to allow swapping out the signaling server client by extending WebrtcProvider and SignalingConn.

  • Creates subscribe, unsubscribe, and publish methods for SignalingConn so external code doesn't need to know how to format signaling server messages.
  • Exports publishSignalingMessage so classes derived from SignalingConn can make use of it, as well as other parts of y-webrtc.
    • Would it be better to move this logic into the publish method and refactor other y-webrtc code that calls it?
  • Creates a connected method, since clients may determine signaling server connectivity differently.
  • Moves websocket client initialization into a setupClient method which can be overridden to use a different signaling server client (ex: Azure Web PubSub client) including differing message event names (ex: message vs group-message)
  • Moves the rooms iteration into a handleConnect method, so that global set does not have to be exported and used in derived classes.
  • Creates a handleMessage method so the logic can be reused and does not need to be included in the client message event definitions of derived classes.

Question

Overriding WebrtcProvider just to change the class used for signaling connections seems excessive. Is there a way we can pass this into the base WebrtcProvider instead?

This is the only line we need to change in the connect() method:
const signalingConn = map.setIfUndefined(signalingConns, url, () => new SignalingConn(url))

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

Successfully merging this pull request may close these issues.

1 participant