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

Updated websocket example to improve clarity and to be more distinct from chat example #1637

Merged
merged 11 commits into from
Jan 8, 2023

Conversation

alexpyattaev
Copy link
Contributor

Motivation

The web sockets example was not looking nice, it was not user-friendly at all (took a while to figure out what it was even supposed to be able to do). Furthermore, it was not actually showcasing anything about web sockets that the chat example did not already demonstrate better. Either it had to go, or it had to be made more distinct, so I took the time to make it more "involved".

Solution

  • all parts of the example have received additional comments
  • the sample webpage has received hints that the user should open browser's console to see messages (it was just blank)
  • added a Python script that demonstrates some more tricky features such as ping/pong handling, and allows to quickly connect with multiple "users" to the test server
  • added the logic to push unsolicited messages to the client form the server (which is not obvious at all though easy to actually do)
  • added comments explaining that one should not try to implement pong responder (as axum already has one builtin, and thus even though you can see the ping messages, you should not actually respond to them)

… version of Chat.

Added more descriptive comments and examples of various data types that can be shipped over websockets.
…l as proper documentation for Ping/Pong mechanism.
@davidpdrsn
Copy link
Member

davidpdrsn commented Dec 13, 2022

Thanks! I skimmed the changes and they generally look good. Will do a more thorough review later.

However we cannot merge Python code. I don't know Python so don't wanna take on maintenance of that. Can you write the client in Rust instead?

@alexpyattaev
Copy link
Contributor Author

Well websocket client in Rust might be a bit more "fun" than I originally anticipated, but I can give it a try. Stay tuned.

@alexpyattaev
Copy link
Contributor Author

All right, now there is a proper Rust client, which is properly illustrating the concurrency features of axum websockets.
Code-wise it is actually 90% identical to the server, which is pretty cute if someone wants to eventually have both sides of the socket in Rust.
Feel free to kill the Python client.py if you feel like it deserves it =)

examples/websockets/client.py Outdated Show resolved Hide resolved
examples/websockets/assets/script.js Outdated Show resolved Hide resolved
examples/websockets/assets/script.js Outdated Show resolved Hide resolved
examples/websockets/src/main.rs Outdated Show resolved Hide resolved
examples/websockets/src/main.rs Outdated Show resolved Hide resolved
examples/websockets/src/main.rs Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
examples/websockets/Cargo.toml Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
alexpyattaev and others added 2 commits December 16, 2022 00:41
All the "trivial suggestions" from review. More substantial refinement to follow.

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@alexpyattaev
Copy link
Contributor Author

Ok the fat trimming is done, I hope I have not missed anything.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Great! Almost there. Just a couple of minor nitpicks here and there.

examples/websockets/assets/index.html Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
examples/websockets/src/client.rs Outdated Show resolved Hide resolved
examples/websockets/src/main.rs Outdated Show resolved Hide resolved
alexpyattaev and others added 2 commits December 17, 2022 00:30
Applied all suggestions as is, pending testing.

Co-authored-by: David Pedersen <david.pdrsn@gmail.com>
@alexpyattaev
Copy link
Contributor Author

Thanks for the tips! I've re-tested everything (there was a missing import and one unneeded import) and the last commit should contain a fully functional example with all your suggestions incorporated.

@alexpyattaev
Copy link
Contributor Author

Bump =)

@davidpdrsn
Copy link
Member

No need for bumps. We'll get to this when we get to it. I'm currently on Christmas holiday playing video games.

Copy link
Member

@davidpdrsn davidpdrsn left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@davidpdrsn davidpdrsn enabled auto-merge (squash) January 5, 2023 10:08
@davidpdrsn davidpdrsn merged commit 8d92902 into tokio-rs:main Jan 8, 2023
@alexpyattaev
Copy link
Contributor Author

You are welcome! I quite like your standards of quality, gives me confidence using Axum in production.

@FrankReh
Copy link
Contributor

I'm sure many people are going to benefit from this example, including the command-line client. Big thank you to both!

I was wondering how to run the example and then I saw the comments in main.rs (of course). Small nit, at least for me to get them running:

diff --git a/examples/websockets/src/main.rs b/examples/websockets/src/main.rs
index 26a31a2..6d598b1 100644
--- a/examples/websockets/src/main.rs
+++ b/examples/websockets/src/main.rs
@@ -3,13 +3,13 @@
 //! Run the server with
 //!
 //! ```not_rust
-//! cargo run -p example-websockets
+//! cargo run -p example-websockets --bin example-websockets
 //! firefox http://localhost:3000
 //! ```
 //!
 //! Alternatively you can run the rust client with
 //! ```not_rust
-//! cargo run -p example-client
+//! cargo run -p example-websockets --bin example-client
 //! ```

@davidpdrsn
Copy link
Member

@FrankReh good catch! Wanna submit a PR?

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.

None yet

4 participants