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

Add protobuf module as wasm module #2054

Draft
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

DmitryAstafyev
Copy link
Collaborator

Description

To avoid generating a code for both (rust and JS) sides, we can:

  • generate only a rust representation of the protocol.
  • create wasm module with generated code
  • use wasm module on JS side to decode/encode messages

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.

@DmitryAstafyev
Copy link
Collaborator Author

General note: code isn't documented at all. It will be before merging.

Copy link
Member

@AmmarAbouZor AmmarAbouZor left a 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"));
Copy link
Member

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.

Comment on lines +1 to +5
use crate::*;
use extend::extend;
use prost::Message;
use serde_wasm_bindgen::{from_value, to_value};
use wasm_bindgen::prelude::*;
Copy link
Member

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.

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.

3 participants