Implement shared custom data (re-worked) based on the previous work from kitgxrl #435

thebashpotato · 2024-05-14T19:30:55Z

Greetings, just wanted to say this is a great library and you've done fantastic work.
Just wanted to throw my hat in the ring to solve the shared data issue.

I read the discussion on kitgxrl's PR and tried to solve the comments you mentioned.
I decided to go with the naming convention of 'transmitter' as that's really what's going on.
I stayed with Arc, as a Fat pointer caused way too many lifetime issues, although I did give it the good ol' college try, in the end an Arc isn't that much overhead.

I also fixed all the old clippy warnings that were causing the make clippy command to complain. This included removing the two client/client.rs modules as its a module naming repetition warning. I just put the code in the corresponding client/mod.rs module instead.

I wrote two comprehensive example clients for both async and raw clients and added proper documentation.

I also added an error, and went with try_transmitter which returns a Result, to ensure the library can't panic.

If there is anything else you would like done, please let me know.

Added a fairly indepth example client.

There were many warnings causing the `make clippy` command to fail. Some of the more noticable changes are moving the client.rs code to the corresponding mod.rs and deleting the former. This was to stop the clippy module inception warning. All unit tests, doc tests, and examples are passing.

kitgxrl · 2024-05-15T08:12:00Z

@thebashpotato I'm a bit confused about the naming convention of Transmitter, could you explain a bit more? Sorry if this is a bit of a silly question. Seems like this is assuming that this is some sort of channel which isn't necessarily true. For my use case for example, this is used more as a way of shared state than just having a channel, it just happen to be one of the issues some had that implementing custom data would fix.

1c3t3a

Hey @thebashpotato thanks a lot for PR!! I really like the channel design here and I agree with the name transmitter! I did a first pass over this code and gave some feedback, also I enabled CI runs.

@kitgxrl: Since we now have two PRs with the same content, let me think about how we proceed.

1c3t3a · 2024-05-15T12:56:53Z

.gitignore

@@ -1,4 +1,5 @@
 target
+target_ra


Could you explain where is this coming from? :) I assume it's not a cargo/rust folder?

Sorry I forgot to remove that, my neovim rust-analyzer setup creates a different folder for the analyzation to speed up development and avoid locks on the target directory. It becomes useful in large Rust codebases.

1c3t3a · 2024-05-15T12:57:53Z

engineio/src/client/client.rs

@@ -1,662 +0,0 @@
-use super::super::socket::Socket as InnerSocket;


Could you please keep this code in client.rs? I assume this was to fix the client/client clippy lint? :)

Yes, it was to fix the lint, I figured since the lint was enabled that it just hadn't got around to being fixed yet, I can put the code back, and disable the lint.

1c3t3a · 2024-05-15T13:00:47Z

socketio/examples/async_transmitter.rs

+ let client = SocketIOClientBuilder::new(url)
+ .namespace("/admin")
+ .on("test", |payload: Payload, socket: SocketIOClient| {
+ async move {


Can't we put the async move after the closue, e.g.:
.on("test", |payload: Payload, socket: SocketIOClient| async move {? That way we have one nesting less.

I tried that and it works, but cargo fmt will move it back to inside the closure lol

1c3t3a · 2024-05-15T13:20:13Z

socketio/examples/async_transmitter.rs

+ Payload::Text(values) => {
+ if let Some(value) = values.first() {
+ if value.is_string() {
+ socket
+ .try_transmitter::<mpsc::Sender<String>>()
+ .map_or_else(
+ |err| eprintln!("{}", err),
+ |tx| {
+ tx.send(String::from(value.as_str().unwrap()))
+ .map_or_else(
+ |err| eprintln!("{}", err),
+ |_| {
+ println!(
+ "Data transmitted successfully"
+ )
+ },
+ );
+ },
+ );
+ }
+ }
+ }


I am not a huge fan of the nesting tbh :D Could we either (or both):

Seperate this out into it's own async function?

Since we're only interested in Payload::Text, maybe we can shrink this further by having if let Payload::Text(values) = payload {

1c3t3a · 2024-05-15T13:24:21Z

socketio/src/asynchronous/client/async_client.rs

+ /// async move {
+ /// match payload {
+ /// Payload::Text(values) => {
+ /// if let Some(value) = values.first() {
+ /// if value.is_string() {
+ /// socket.try_transmitter::<mpsc::Sender<String>>().map_or_else(
+ /// |err| eprintln!("{}", err),
+ /// |tx| {
+ /// tx.send(String::from(value.as_str().unwrap()))
+ /// .map_or_else(
+ /// |err| eprintln!("{}", err),
+ /// |_| println!("Data transmitted successfully"),
+ /// );
+ /// },
+ /// );
+ /// }
+ /// }
+ /// }
+ /// Payload::Binary(bin_data) => println!("{:#?}", bin_data),
+ /// #[allow(deprecated)]
+ /// Payload::String(str) => println!("Received: {}", str),
+ /// }
+ /// }
+ /// .boxed()


Same feedback as in the example :)

1c3t3a · 2024-05-15T13:25:38Z

socketio/src/asynchronous/client/builder.rs

+ ///
+ /// # Example
+ ///
+ /// ```no_run


Why no_run? I assume it disables doc tests? Does it compile them?

1c3t3a · 2024-05-15T13:26:04Z

socketio/src/client/client.rs

@@ -1,478 +0,0 @@
-use std::{


Same comment as for the other client, I'd like to keep it, for now!

1c3t3a · 2024-05-15T13:26:26Z

socketio/src/client/raw_client.rs

+ /// match payload {
+ /// Payload::Text(values) => {
+ /// if let Some(value) = values.first() {
+ /// if value.is_string() {
+ /// socket.try_transmitter::<mpsc::Sender<String>>().map_or_else(
+ /// |err| eprintln!("{}", err),
+ /// |tx| {
+ /// tx.send(String::from(value.as_str().unwrap()))
+ /// .map_or_else(
+ /// |err| eprintln!("{}", err),
+ /// |_| println!("Data transmitted successfully"),
+ /// );
+ /// },
+ /// );
+ /// }
+ /// }
+ /// }
+ /// Payload::Binary(bin_data) => println!("{:#?}", bin_data),
+ /// #[allow(deprecated)]
+ /// Payload::String(str) => println!("Received: {}", str),
+ /// }
+ /// };


Same feedback as for the rest of similar code.

kitgxrl · 2024-05-15T14:13:13Z

@1c3t3a Sure! Although, I'm still a bit confused with transmitter as the naming here, honestly.

thebashpotato · 2024-05-15T14:40:13Z

@1c3t3a I will revise this PR to resolve the issues you've raised. Also as far as the clippy lints go, you can define them in the toplevel Cargo.toml in workspaces now, and each member workspace Cargo.toml can inherit them. I can add that functionality if you would like. Currently the lints are enabled and disabled at seemingly random parts of the codebase.

@kitgxrl The naming convention of Transmitter was taken directly from mpsc::channel crate.
The sender in this situation usually denoted by the tx variable naming stands for transmitter. The channel approach in my view is much more flexible and powerful in more complex situations. Basically, the sender is passed into the client itself, leaving the reciever unencumbered to recieve messages where ever the user sees fit, most notably, outside of the callbacks, this is very usefull in design in extremely large protocols, which is what I'm working on.

kitgxrl · 2024-05-15T14:44:44Z

@thebashpotato Sure, however you're able to store arbitrary data here, which was my original intention. transmitter I feel is a bit misleading of how it can actually be used. custom_data or something similar still feels a bit better as it's made clear arbitrary data is able to be stored, not just a channel sender.

thebashpotato · 2024-05-15T14:49:21Z

@kitgxrl Fair enough, I leave the decision up to @1c3t3a as the repo owner. I'll name it whatever, in the mean time I'll continue fixing the other issues.

Put client.rs modules back, and explicitly allow the clippy warnings.

thebashpotato · 2024-05-16T15:28:18Z

@1c3t3a Ok, PR is updated with your recommendations.

shenjackyuanjie · 2024-05-18T03:56:55Z

HI!
I like the idea of this PR also
at the same time, I'm trying to implement Custom parser for socketio Packet
so, there may be some part of code need to be changed later for supporting parser

socketio/src/packet.rs

thebashpotato added 3 commits May 14, 2024 11:53

Add transmitter sender data layer for asynchronous client

a2e42de

Added a fairly indepth example client.

Add transmitter sender data layer for raw client

b386b0c

1c3t3a reviewed May 15, 2024

View reviewed changes

Refactor example code and documentation.

82d5703

Put client.rs modules back, and explicitly allow the clippy warnings.

shenjackyuanjie reviewed May 18, 2024

View reviewed changes

socketio/src/packet.rs Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement shared custom data (re-worked) based on the previous work from kitgxrl #435

Implement shared custom data (re-worked) based on the previous work from kitgxrl #435

thebashpotato commented May 14, 2024 •

edited

Loading

kitgxrl commented May 15, 2024

1c3t3a left a comment

1c3t3a May 15, 2024

thebashpotato May 15, 2024 •

edited

Loading

1c3t3a May 15, 2024

thebashpotato May 15, 2024

1c3t3a May 15, 2024

thebashpotato May 15, 2024 •

edited

Loading

1c3t3a May 15, 2024

1c3t3a May 15, 2024

1c3t3a May 15, 2024

1c3t3a May 15, 2024

1c3t3a May 15, 2024

kitgxrl commented May 15, 2024

thebashpotato commented May 15, 2024

kitgxrl commented May 15, 2024

thebashpotato commented May 15, 2024

thebashpotato commented May 16, 2024

shenjackyuanjie commented May 18, 2024

		@@ -1,662 +0,0 @@
		use super::super::socket::Socket as InnerSocket;

Implement shared custom data (re-worked) based on the previous work from kitgxrl #435

Are you sure you want to change the base?

Implement shared custom data (re-worked) based on the previous work from kitgxrl #435

Conversation

thebashpotato commented May 14, 2024 • edited Loading

kitgxrl commented May 15, 2024

1c3t3a left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebashpotato May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thebashpotato May 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitgxrl commented May 15, 2024

thebashpotato commented May 15, 2024

kitgxrl commented May 15, 2024

thebashpotato commented May 15, 2024

thebashpotato commented May 16, 2024

shenjackyuanjie commented May 18, 2024

thebashpotato commented May 14, 2024 •

edited

Loading

thebashpotato May 15, 2024 •

edited

Loading

thebashpotato May 15, 2024 •

edited

Loading