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 messages #62

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

Conversation

byrond
Copy link

@byrond byrond commented Dec 8, 2023

Issue #11 provides a solution for using a different type of signaling server. However, certain parts of y-webrtc make assumptions about the messages being sent through the signaling server. This is a suggestion for refactoring some of the code to make it possible to swap out SignalingConn with a derived class.

In our case, we wanted to use Azure Web PubSub as the signaling server, and Microsoft uses slightly different message formats, event names, and terminology (i.e. "join groups" instead of "subscribe to topics", "group-message" instead of a "message" event, etc.).

Here is our example code where we have derived classes from SignalingConn and WebrtcProvider using the refactored y-webrtc in this PR. It uses the Azure Web PubSub client from Microsoft.

export class AzureWebPubSubSignalingConn extends SignalingConn {
  private clientConnected = false;

  private webPubSubClient!: WebPubSubClient;

  setupClient() {
    this.webPubSubClient = new WebPubSubClient(this.url);
    this.webPubSubClient.on('connected', (e) => {
      this.clientConnected = true;
      console.log(`connected (${this.url}) with ID ${e.connectionId}`);
      this.handleConnect();
    });
    this.webPubSubClient.on('disconnected', (e) => console.log(`disconnect (${this.url}): ${e.message}`));
    this.webPubSubClient.on('stopped', () => console.log(`stopped (${this.url})`));
    // Set an event handler for group messages before connecting, so we don't miss any.
    this.webPubSubClient.on('group-message', (e) => {
      this.handleMessage(e.message.group, e.message.data);
    });
    // Connect to the signaling server.
    this.webPubSubClient.start();
  }

  connected() {
    return this.clientConnected;
  }

  subscribe(roomName: string) {
    this.webPubSubClient.joinGroup(roomName);
  }

  unsubscribe(roomName: string) {
    this.webPubSubClient.leaveGroup(roomName);
  }

  publish(roomName: string, message: string | object) {
    let messageType: WebPubSubDataType = 'json';
    if (typeof message === 'string') {
      messageType = 'text';
    }
    this.webPubSubClient.sendToGroup(roomName, message, messageType);
  }

  destroy() {
    this.webPubSubClient.stop();
  }
}

export class AzureWebPubSubSignalingWebrtcProvider extends WebrtcProvider {
  connect() {
    this.shouldConnect = true;
    this.signalingUrls.forEach((url) => this.addSignalingConn(url, () => new AzureWebPubSubSignalingConn(url)));
    if (this.room) {
      this.room.connect();
    }
  }
}

The following refactoring was done:

  • Decouples SignalingConn from WebsocketClient and creates a property to hold an instance of it.
  • Creates subscribe, unsubscribe, and publish methods for SignalingConn so external code doesn't need to know how to format signaling server messages.
  • 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.

Questions

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? Maybe create a factory method that we can override in a derived class.

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

What about this needs to be cleaned up to prevent introducing TypeScript errors if/when y-webrtc.js is converted to TS?

@@ -666,4 +696,10 @@ export class WebrtcProvider extends Observable {
})
super.destroy()
}

addSignalingConn(url, conn) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

conn is technically a function. Maybe we need to call it createT or something similar?

const topics = Array.from(rooms.keys())
topics.forEach(topic =>
this.subscribe(topic)
)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Azure Web PubSub cannot join multiple groups, but that shouldn't stop the built-in client from doing so. Maybe instead of the forEach this needs a subscribeMulti() method or let subscribe() detect whether topic is an array in derived classes and do the iteration there?

@byrond
Copy link
Author

byrond commented Apr 15, 2024

@dmonad do you have any feedback on this approach or if it is even feasible to do this level of refactoring in your module?

@dmonad
Copy link
Member

dmonad commented Apr 16, 2024

Hi @byrond,

first of all, thanks for contributing to this repository!

I manage a lot of different tools, and I'm always hesitant to merge new features.

I prefer simplicity over extensibility because simple projects are easier to maintain. Any new feature that is introduced causes at least one more bug that I have to fix. At this time, this feature is not a priority for me.

That said, I think the approach is valid. I haven't done a full review yet. If you gather more support, I might merge this feature into the next major release.

Btw, this repository is fully typed using jsdoc comments (I prefer that over typescript files). If you do npm run lint it should give you type errors (not just syntax errors).

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.

2 participants