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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thebashpotato
Copy link

@thebashpotato thebashpotato commented May 14, 2024

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
Copy link

kitgxrl commented May 15, 2024

@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.

Copy link
Owner

@1c3t3a 1c3t3a left a comment

Choose a reason for hiding this comment

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

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.

.gitignore Outdated
@@ -1,4 +1,5 @@
target
target_ra
Copy link
Owner

Choose a reason for hiding this comment

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

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

Copy link
Author

@thebashpotato thebashpotato May 15, 2024

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

Copy link
Author

@thebashpotato thebashpotato May 15, 2024

Choose a reason for hiding this comment

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

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

Comment on lines 32 to 53
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"
)
},
);
},
);
}
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

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

  1. Seperate this out into it's own async function?
  2. Since we're only interested in Payload::Text, maybe we can shrink this further by having if let Payload::Text(values) = payload {

Comment on lines 123 to 146
/// 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()
Copy link
Owner

Choose a reason for hiding this comment

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

Same feedback as in the example :)

///
/// # Example
///
/// ```no_run
Copy link
Owner

Choose a reason for hiding this comment

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

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

@@ -1,478 +0,0 @@
use std::{
Copy link
Owner

Choose a reason for hiding this comment

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

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

Comment on lines 89 to 110
/// 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),
/// }
/// };
Copy link
Owner

Choose a reason for hiding this comment

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

Same feedback as for the rest of similar code.

@kitgxrl
Copy link

kitgxrl commented May 15, 2024

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

@thebashpotato
Copy link
Author

@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
Copy link

kitgxrl commented May 15, 2024

@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
Copy link
Author

@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
Copy link
Author

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

@shenjackyuanjie
Copy link
Contributor

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

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.

4 participants