-
Notifications
You must be signed in to change notification settings - Fork 39
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
Add protobuf module as wasm module #2054
base: master
Are you sure you want to change the base?
Conversation
General note: code isn't documented at all. It will be before merging. |
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.
Wow this a hug and -at the same time- very clean PR 💯
I had a quick look on the rust part and wrote some comments about the point that caught my eyes. Most of them are points for the final phase if the other performance issues are resolved, and some of them could bring tiny improvements for the performance.
/// * `RangeInclusive` | ||
/// | ||
pub mod common { | ||
include!(concat!(env!("OUT_DIR"), "/common.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.
I don't know if the backslash will be working on Windows, I think we can test that on Windows first before adjusting it manually. Hopefully the rust compiler can resolve automatically.
use crate::*; | ||
use extend::extend; | ||
use prost::Message; | ||
use serde_wasm_bindgen::{from_value, to_value}; | ||
use wasm_bindgen::prelude::*; |
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 think we can at some point include the use statements inside the macro itself by writing the full paths to the types inside it.
This would make any future changes easier by changing the macro itself and not having to add using statement for each file at a time.
application/apps/rustcore/rs-bindings/src/js/converting/observe.rs
Outdated
Show resolved
Hide resolved
application/apps/rustcore/rs-bindings/src/js/converting/observe.rs
Outdated
Show resolved
Hide resolved
application/apps/rustcore/rs-bindings/src/js/jobs/converting.rs
Outdated
Show resolved
Hide resolved
5cc8772
to
92fc528
Compare
92fc528
to
94d7357
Compare
94d7357
to
b726d65
Compare
19cab48
to
c3ef80b
Compare
c3ef80b
to
a1f5604
Compare
Description
To avoid generating a code for both (rust and JS) sides, we can:
In this way we can be sure: that messages on rust and JS will be encoded/decoded in the same way
Motivation
Research shows: that using different libraries/modules to generate code based on
*.proto
at the end can give different results. For example same message can be encoded into different bytes on JS side and on Rust side. That makes work with protocol unpredictable.