-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Rust port of mediasoup! #518
Conversation
…without awkward imports
…nse, more careful visibility settings
… closing router on worker side, some trivial methods on Router, methods comments
…ne on Worker and Router instead
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.
Amazing. I must do a proper review and it will take some days before I finish. For now just a few comments:
- I'd like to move current Travis-CI to GitHub CI entirely and not just for Rust.
- I don't like the idea of including frontend examples in
mediasoup/rust/
folder. - I don't understand why there is
rust/.gitignore
instead of just adding entries to main.gitignore
. - Why is mediasoup rust version 0.5.0?
Files for Rust and Node.js will most likely be different, so one doesn't depend on the other. I will try to find time for Node.js workflow in a separate PR. Here I just didn't want to submit Rust version without CI in place from the beginning.
There is a convension in Rust (supported by Cargo) to have There is no identical backend example in TypeScript so it seems fine to me to have it there. I have no stong feelings about it if you want me to move it somewhere.
Moved
This is less similar to TypeScript library and more similar to mediasoup-client case. I am open to suggestions how to make it clearer which version of the worker/mediasoup Rust version corresponds to (I'm also exploring how to turn worker into Rust crate to it can be installed as a dependency automatically). Updated ticket with similarities/differences with version info. |
Thanks. In fact we are having problems with Travis-CI due to its new limits.
Perfect then :)
Ok, let's do this way for now and let's see in the future. NOTE: I still have to review more things and won't be able to finish this week. Will do next one, promise. |
…d/or payload channel is already closed
Tests are still a bit flaky at times, but I'll probably be ironing them out in next version instead of introducing more changes to this PR. |
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.
Please address just these two comments and let's merge.
…tpAssociation::FillJson()
waiting for tests to pass in CI |
What this massive PR includes:
There are minor cleanups in worker and existing tests, but nothing significant (let me know if that is an issue).
They are primarily related to stricter typing generally used in Rust.
Since there is #397 open and adding CI for Rust into existing setup would be challenging, I decided to use GitHub actions instead, hopefully that is OK.
Code style used is default for Rust ecosystem with no tweaks. For TypeScript code used in echo example (the only one so far) I used already established code style from this repository to make it consistent.
I acknowledge how big this is and would be happy to address any feedback there might be. Please start with documentation on docs.rs, it explains some high-level information about this port.
References:
As discussed with Iñaki, there will be some complementary activities related to this around website documentation and such.