-
Notifications
You must be signed in to change notification settings - Fork 76
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
feat: emit multipe parameters #249
Conversation
Codecov Report
@@ Coverage Diff @@
## main #249 +/- ##
==========================================
+ Coverage 86.27% 86.33% +0.05%
==========================================
Files 31 31
Lines 2478 2562 +84
==========================================
+ Hits 2138 2212 +74
- Misses 340 350 +10
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
0987cdf
to
fdd47fd
Compare
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.
Thanks for the PR! Don't quite understand what the problem was, nor the solution, a brief explanation (preferably in comments) as to what happened may be helpful
I had left an earlier comment but deleted it. Turns out it was an issue on the server side |
This does seem to solve the problem I was experiencing here. Good job |
The root reason is socketio encoding accept revert #220 is ok, but we still need a way to emit simple payload like number and string if nodejs socketio server needs. the new API is a POC, even the name of that API is not good. |
This is not entirely true, we don't need number since this is part of Json, eg: |
I agree, not include number. #220 is a little wired that callback payload can not emit back, maybe |
@david-sailplan can you confirm that use a single json payload is able to emit to a socketio server with |
@SSebo I'm not entirely sure what you're asking here. If you're asking if something to the effect of
works with server code
I can confirm that it does not. Log would be
|
If what you're asking is if
works with the above server code the answer is yes. It behaves as we expected even after removing the Payload::Number |
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.
Just dropping by, as always @1c3t3a 's in charge
263fd9b
to
34275ff
Compare
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.
Thank you for being receptive to comments so far, feel free to ignore the "nit"s you don't want to do, just saying things I noticed.
Changes to Packet's public interface require @1c3t3a 's approval. We MIGHT be able to still squeeze by with changes to Packet, as it's not likely many use it. Your call.
payload: Payload, | ||
event: Event, | ||
nsp: &'a str, | ||
event: Option<Event>, |
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.
nit: when is event none? Only usage I can see is in the tests
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 found client.emit(Buffer.from([4, 5, 6]));
in socket-io.js, I thought event is optional that time. But I lookup the socketio dos, event is not optional. Does this mean the event can be a valid payload?
I could not figure out how to emit like that in rust-socketio
.
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.
right now in rust-socketio
if only receive 1 item in packet.data, it will be treat as payload, not event.
rust-socketio/socketio/src/client/raw_client.rs
Lines 325 to 354 in 4ac27c7
fn handle_event(&self, packet: &Packet) -> Result<()> { | |
// unwrap the potential data | |
if let Some(data) = &packet.data { | |
// the string must be a valid json array with the event at index 0 and the | |
// payload at index 1. if no event is specified, the message callback is used | |
if let Ok(serde_json::Value::Array(contents)) = | |
serde_json::from_str::<serde_json::Value>(data) | |
{ | |
let event: Event = if contents.len() > 1 { | |
contents | |
.get(0) | |
.map(|value| match value { | |
serde_json::Value::String(ev) => ev, | |
_ => "message", | |
}) | |
.unwrap_or("message") | |
.into() | |
} else { | |
Event::Message | |
}; | |
self.callback( | |
&event, | |
contents | |
.get(1) | |
.unwrap_or_else(|| contents.get(0).unwrap()) | |
.to_string(), | |
)?; | |
} | |
} | |
Ok(()) |
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.
Where did you find that? Specification only mentions, that when no event is specified, "Message" should be used. So having an Option<Event>
would not make sense 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.
packet.data = match json_data { | ||
Value::Array(vec) if vec.is_empty() => None, | ||
// Value::Array(vec) if vec.len() == 1 => vec.get(0).map(|v| v.to_owned()), | ||
_ => Some(json_data), |
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.
nit: would this still have the _placeholder
in it?
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.
Yes, _placeholder will be replaced to real Payload::Binary
in socket.rs
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.
packet.rs should handle all de-serialization of the packet. This will become more important if/when we want to support multiple versions.
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.
BinaryEvent
can be mixed with normal data and binary data like client.emit('test', Buffer.from([1, 2, 3]), "4", Buffer.from([5, 6]));
, so we can not just remove all _placeholder
as we do not know where the binary data is in the sequence of received items. socket.rs
will not parse the packet, just to replace _placehoulder
to Payload::Binary
I checked socket.io protocol, for now BinaryEvent is not changed when added in version 2. If we do not want to touch _placehoulder
in socket.rs, we should replaced to Payload::Binary
in packet.rs to keep order of mixed multi payloads. packet.data
can not be Vec<Value>
or Vec<String>
but Vec<Payload>
.
.map(|value| match value { | ||
serde_json::Value::String(ev) => ev, | ||
_ => "message", | ||
}) |
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.
.map(|value| match value { | |
serde_json::Value::String(ev) => ev, | |
_ => "message", | |
}) | |
.and_then(|value| match value { | |
serde_json::Value::String(ev) => Some(ev), | |
_ => None | |
}) |
@@ -158,6 +158,47 @@ impl Socket { | |||
attachments.push(bin_data); | |||
} | |||
|
|||
pub(crate) fn decode_binary_payload( |
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.
nit: why did this move? It's only used in raw_client
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 found encode is in socket.rs, so I move decode in socket.rs too.
Why was this closed @SSebo ? |
Good question! |
js client can emit multiple parameters in one call. user socketio server
socket.on("foo", async (userId, body, callback)=>{})
(#246) accept this kind of data which rust-socektio can not emit.