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

Proposal: handle onMessage by message type #315

Closed
endel opened this issue Feb 21, 2020 · 20 comments · Fixed by #322
Closed

Proposal: handle onMessage by message type #315

endel opened this issue Feb 21, 2020 · 20 comments · Fixed by #322
Milestone

Comments

@endel
Copy link
Member

endel commented Feb 21, 2020

This would be a big breaking change, I'd like to know your thoughts and opinions about this. Will it be valuable/worth doing? Feedback is highly appreciated!

Server-side

client.send() to replace this.send() + message type:

// new
client.send("skill", { name: "fireball" })

// deprecate
this.send(client, { type: "skill", name: "fireball" }

client.send() a schema-encoded message

This is already supported by the server-side. See below plans to support this from client-side as well.

class SkillMessage extends Schema {
  @type("string") name: string;
}

const skill = new SkillMessage();
skill.name = "fireball";
client.send(skill);

Broadcast with type + data

// new
this.broadcast("skill", { name: "fireball" })

// deprecate
this.broadcast({ type: "skill", name: "fireball" })

onMessage to handle messages sent from the clients

// new
onCreate() {
    this.onMessage("move", (client, message) => {
        // handle "move" message
    });

    this.onMessage(MoveMessage, (client, message) => {
        // handle schema-encoded "SchemaMessage"
        // (get autocompletion for "message")
    });
}

// deprecate
onMessage(client, message) {
}

Client-side

Allow sending the type of a message along with its data:

// new
room.send("move", { angle: 270 });

// deprecate
room.send({ type: "move", angle: 170 });

Allow sending a Schema structure from the client-side (this will be specially useful for strongly-typed languages, such as C#, C++, etc.)

class MoveMessage extends Schema {
    @type("number") angle: number;
}

const message = new MoveMessage();
message.angle = 170;

room.send(message);

Receiving messages by type:

// new
room.onMessage("skill", (message) => {
  // handle "skill" message
});

room.onMessage(SkillMessage, (message) => {
  // handle schema-encoded "skill" message
});

// deprecate
room.onMessage((message) => {
  // handle any type of message
});

As always suggestions are welcome.

@endel endel added this to the 1.0.x milestone Feb 21, 2020
@seiyria
Copy link
Contributor

seiyria commented Feb 21, 2020

I personally welcome this change: I had to write my own wrapper to do this, and I believe many others have had to do similar. Having it be baked in would save people from needing to have varied approaches to doing this (which ultimately all accomplish the same thing).

@damian-pastorini
Copy link

Hi @endel , my 2 cents from a newbie dev point of view.

This:

// new
this.broadcast("skill", { name: "fireball" })
// deprecate
this.broadcast({ type: "skill", name: "fireball" })

And this:

// new
room.send("move", { angle: 270 });
// deprecate
room.send({ type: "move", angle: 170 });

Feels really forcing clients to do stuff we sometimes don't need, I understand it could have good impact related to typed code, but if in some cases we just need to send one value, like:
room.send({myVal: 1 }); I would be forced to send a type I just may not need.

Also I'm guessing that would impact on the "traffic" and the amount of data sent?
If try to minimize my traffic as low as possible, in some cases I could be doing something like:
room.send({x: 1 }); and include type would be room.send('x', {x: 1 }); ? is an additional param in every data send.

On the other hand, manage schema classes like this:

// new
room.onMessage(SkillMessage, (message) => {
  // handle schema-encoded "skill" message
});

room.onMessage("skill", (message) => {
  // handle "skill" message
});

// deprecate
room.onMessage((message) => {
  // handle any type of message
});

It looks so much better! You would probably save me so many conditions checking if the message is coming with some specific value to run an action.

@seiyria
Copy link
Contributor

seiyria commented Feb 21, 2020

Feels really forcing clients to do stuff we sometimes don't need, I understand it could have good impact related to typed code, but if in some cases we just need to send one value, like:
room.send({myVal: 1 }); I would be forced to send a type I just may not need.

It definitely means this can be typed more strongly, which is good- you can force a message of a type to have certain values, which is good for developers, and sanity.

Additionally, if you are doing something like room.send({ val: 1 }), how does your client know what to do with it? You implicitly have a type there that you're just not sending, but you're still doing the same thing you would if you had a type, only it makes more sense.

The real bandwidth saves don't come from cutting a small type property (a max of a few bytes per message), but rather from not sending large amounts of data.

@endel
Copy link
Member Author

endel commented Feb 21, 2020

Regarding the number of bytes being sent, room.send('x', 1) would actually result in less bytes than room.send({x: 1}) 👀

@damian-pastorini
Copy link

Thanks for the clarification @seiyria , in the commented case the client could know what to do depending on the attribute sent, room.send({ val: 1 }) > could be room.send({ x: 1 }) or room.send({ y: 1 }), that's what I mean, and the same could be applied for broadcast, I could just brodcast({o:12}), and the client could remove the "o" (for object) with ID 12.
@endel now room.send('x', 1) makes much more sense, I was thinking that I was forced to send the second parameter as an object as well in the send method, like in the first example: room.send('x', {x: 1}), if I would be able to just send the type and the value like you just commented room.send('x', 1) that's amazing 😍

@damian-pastorini
Copy link

I'll better be mentally preparing for another full refactor on my app 😅

@Zielak
Copy link
Contributor

Zielak commented Feb 21, 2020

That'll sure be breaking change for me, but I'll accept it. I'm sending "type" values with every event anyway :)

@stats
Copy link

stats commented Feb 23, 2020

I like this change.

@endel endel pinned this issue Feb 23, 2020
@krazyjakee
Copy link

So let's say I have a lot of these around my application, presumably at the moment ALL callback functions are called for every message. Could this mean an optimisation where the callback fires only when that message type is received?

@liana-p
Copy link

liana-p commented Feb 24, 2020

I'm already kind of doing this manually in my game, so having an "official" way to handle message types would be nice.

It should probably be optional for those who want to send very small efficient messages though...

Also bonus point if it can be a number so we can use enums to reduce bandwidth usage, otherwise including a string type on every message increases the size of the message.

@Bluebang
Copy link

cool😁

@zgz682000
Copy link

It sounds like an amazing change, and I would more like to use Schema instead of Fossil Delta. Why I choose Fossil Delta but not Schema? Because the schema code generator is not customable. Would you consider to implement the generator by template mechanism?

@endel
Copy link
Member Author

endel commented Mar 2, 2020

Hi @zgz682000, you mind elaborating the "template mechanism" on a new issue? Cheers!

@AJ92
Copy link

AJ92 commented Mar 3, 2020

i also support the change :)

maybe the old public interface can be kept? it can just call the new stuff where ever possible so not everything has to be refactored ?

@zgz682000
Copy link

Hi @zgz682000, you mind elaborating the "template mechanism" on a new issue? Cheers!

of cause, I will write my advice in the schema repository after have a research of it

@Bluebang
Copy link

Bluebang commented Mar 3, 2020

Can you consider adding a one-time request method?

//Current implementation methods
//client side
function JustDoOnce(){
    client.send(getSysCfg,{});
    client.onMessage(getSysCfg, (message) => {
        client.removeListener(getSysCfg);
        //dosomething
    });
}

//My suggestion
//client side
function JustDoOnce(){
    client.send(getSysCfg,{}).then(data=>{
        //dosomething
    })
}

Can I do this?😁

endel added a commit to colyseus/colyseus.js that referenced this issue Mar 14, 2020
endel added a commit that referenced this issue Mar 14, 2020
endel added a commit that referenced this issue Mar 14, 2020
endel added a commit to colyseus/colyseus.js that referenced this issue Mar 14, 2020
endel added a commit to colyseus/colyseus-defold that referenced this issue Mar 14, 2020
endel added a commit to colyseus/colyseus-defold that referenced this issue Mar 15, 2020
endel added a commit that referenced this issue Mar 15, 2020
endel added a commit to colyseus/colyseus-unity-sdk that referenced this issue Mar 21, 2020
endel added a commit to colyseus/colyseus.js that referenced this issue Mar 29, 2020
endel added a commit that referenced this issue Mar 29, 2020
endel added a commit to colyseus/colyseus-defold that referenced this issue Mar 29, 2020
@endel endel mentioned this issue Apr 11, 2020
17 tasks
endel added a commit to colyseus/colyseus-cocos2d-x that referenced this issue Apr 19, 2020
endel added a commit to colyseus/colyseus-cocos2d-x that referenced this issue Apr 19, 2020
endel added a commit to colyseus/colyseus-cocos2d-x that referenced this issue Apr 19, 2020
endel added a commit that referenced this issue Apr 22, 2020
* wip restructuring onMessage

* refactoring onMessage and Transport for better message type support. #315

* refactoring protocol messages, client.send(), and client.raw()

* refactoring Protocol's send / getMessageBytes #48

* refactoring room.broadcast() and serializers to use client.raw()

* fixes integration tests #315

* fixes WebSocketClient tests. #315

* bump alpha version

* use enqueueRaw for client.send

* improve onMessage tests #315

* enhancement: swap old Client 'ref' after re-connection is successful

* bump 0.13.0-alpha.4

* feat: allow send/receive messages by type without payload. #315

* update test name #315

* feat: generateId() not accepts optional 'length' argument.

* enhancement: expose different error codes for internal errors. export ServerError. #317

* fix silly mistake during refactoring

* fix: force onCreate to be implemented by end user. #319

* revert abstract onCreate. fixes strict usage on .define() method. #319

* lobby: add LobbyRoom and .enableRealtimeListing() feature.

* add 'name' to player on RelayRoom. forward messages in array format

* new alpha version

* bump 0.13.0
endel added a commit to colyseus/colyseus-unity-sdk that referenced this issue Apr 22, 2020
* fixed source style of WebSocket, added forst working message queue

* rewriting OnMessage to support messages by type

* use casting on Action<> to return typed value colyseus/colyseus#315

* initial version for typed room.OnMessage(). refactor error codes. #113

* implement Send() and OnMessage(). closes #113

* fixes for webgl build

* remove deprecated 'enqueuedCalls' for Connection. fixes conditional compilation for WEBGL

* fix: allow receiving messages without payload

* avoid re-allocating bytes when decoding msgpack objects

Co-authored-by: AJ <aj.3dev@gmail.com>
@endel endel unpinned this issue Apr 22, 2020
@zgz682000
Copy link

0.13.x seems not implement the usage like thisthis.onMessage(MoveMessage, (client, msg)=>{}). Is it still in you future plan?

@endel
Copy link
Member Author

endel commented Jun 16, 2020

Hi @zgz682000, only the client-side is able to listen to Schema messages.

The client-side is not allowed to send Schema messages so far, because only the JavaScript client has the encoder implemented. Writing the Schema encoder on other languages would be a huge effort.

@zgz682000
Copy link

Thank you for response immediately. may it be used as a optional way of listening message, even if temporarily provide for JS/TS ?

@zgz682000
Copy link

although I don't know why I hope this, maybe just for looks cool... or... comfortable...

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 a pull request may close this issue.

10 participants