Skip to content
This repository has been archived by the owner on Nov 7, 2020. It is now read-only.

add generic message handler to transport #428

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add generic message handler to transport #428

wants to merge 1 commit into from

Conversation

thehunmonkgroup
Copy link
Contributor

As far as I could tell, the transport has no way to deal with generic messages
that don't deal with state changes or auth. I found this handy to broadcast
stateless commands to all an event's connected users.

Example usage...

Send message via transport:

trans.send("foo", {
  id: some_id,
  roomId: event.getRoomId(),
});

Re-broadcast in room manager:

mgr.on("foo", _.bind(function(socket, args) {
    var event = this.getEvent(args.roomId);
    if (event && socket.user) {
        if(socket.user.isAdminOf(event)) {
            mgr.broadcast(event.getRoomId(), "message", {
              type: "foo-message",
              data: {
                id: args.id,
              },
            });
            mgr.writeAck(socket, "foo", args);
            return;
        }
    }
    mgr.writeErr(socket, "foo");
}, this));

Register callback to catch broadcast:

var callback = function(args) {
    // Do something with args.id
}
trans.registerMessageCallback("foo-message", callback);

@yourcelf
Copy link
Collaborator

yourcelf commented Sep 1, 2015

Generally looks fine to me; though I'd like it much better if there were tests.

One thing I do notice: the implementation only allows a single listener to be specified for each message, though it has the flavor of an EventEmitter type interface that would allow multiple. It might be worth exploring whether mixing in an EventEmitter type thing (perhaps using Backbone.Events would be cleaner and more robust.

As far as I could tell, the transport has no way to deal with generic messages
that don't deal with state changes or auth. I found this handy to broadcast
stateless commands to all an event's connected users.
@thehunmonkgroup
Copy link
Contributor Author

Yep, love the idea of using Backbone.Events, patch updated accordingly.

I will note I realized that there is basic support for generic events already, because of these lines of code in this.sock.onmessage():

        // Trigger all messages, even if we've handled them above.
        this.trigger(msg.type, msg.args);

However, I still like the idea of a specific message type for generic messages -- it somehow seems messy to leverage those lines of code for this purpose.

Regarding tests: I'm not opposed to writing them, and, I've burned many hours of my time just trying to understand the core code -- I've been resistant to the time investment I imagine from learning the testing framework as well. Would you be willing to give me some guidance to help get up to speed? In particular, a few examples of existing tests that I can use as a template for testing this new functionality.

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

Successfully merging this pull request may close these issues.

2 participants