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

[WIP] WASM support with grpc-web #90

Closed
wants to merge 2 commits into from

Conversation

shravanshetty1
Copy link

@tony-iqlusion messed up the rebase

…nabled grpc by default since it should be wasm compatible now
@shravanshetty1
Copy link
Author

@tony-iqlusion Cannot remove the grpc feature since it conditionally compiles some grpc related code, which is needed for grpc_web. I think this breaks normal grpc clients and only supports grpc_web. Please review

@shravanshetty1 shravanshetty1 changed the title WASM support with grpc-web [WIP] WASM support with grpc-web Jun 23, 2021
@tony-iqlusion
Copy link
Member

@shahankhatch looks good. want to remove [WIP]?

@shravanshetty1
Copy link
Author

@shahankhatch looks good. want to remove [WIP]?

Unfortunately after looking more closely at the code thats being deleted. I can confidently say that this will only support grpc_web, normal grpc wont work. Will test it out when i get some time.

I am not sure whats the best solution here, we could make a seperate directoy called "cosmos-sdk-proto-wasm" or another solution could be to wait for "tonic" to enable grpc_web using some kind of feature which will be tracked in issue hyperium/tonic#645. People who want wasm grpc-web till than can use my fork.

@tony-iqlusion
Copy link
Member

@shravanshetty1 you can conditionally gate how the tonic dependency is included depending on whether or not it's a WASM target:

https://github.com/cosmos/cosmos-rust/blob/main/cosmos-sdk-rs/Cargo.toml#L27-L28

@shravanshetty1
Copy link
Author

@shravanshetty1 you can conditionally gate how the tonic dependency is included depending on whether or not it's a WASM target:

https://github.com/cosmos/cosmos-rust/blob/main/cosmos-sdk-rs/Cargo.toml#L27-L28

By disabling tonic, we can get wasm compatibility but we cant use grpc_web. To use grpc_web we will have to not disable tonic, but only disable the transport feature of tonic. This will make the generated code wasm compatible and allow us to use grpc_web using clients such as https://github.com/titanous/grpc-web-client.

However, since the generated code without transport feature of tonic wont allow us to use normal grpc and only force us to use grpc_web.

We could generate 2 cosmos-sdk-proto, one which is wasm+grpc_web(cosmos-sdk-proto-wasm) compatible and the other is the existing one.
We could also just wait for tonic to support grpc_web. After they support it the tonic generated code will have compilation flags which will allow us to compile for grpc_web wasm and normal based on situation.

Its your call.

@tony-iqlusion
Copy link
Member

Aah, I guess this is where we're running into drawbacks with having pre-generated/checked-in code, in that we can't specialize things to a particular platform.

I guess for now if you have a working fork that's fine, and we should perhaps circle back on this issue after the next release (it'd be good to get one out ASAP as there's a new version of tendermint-rs)

@shravanshetty1
Copy link
Author

For any future readers - you can find wasm+grpc_web compatible version https://github.com/shravanshetty1/cosmos-rust/tree/wasm2

@shravanshetty1 shravanshetty1 reopened this Jul 3, 2021
@shravanshetty1 shravanshetty1 changed the title [WIP] WASM support with grpc-web WASM support with grpc-web Jul 3, 2021
@shravanshetty1 shravanshetty1 changed the title WASM support with grpc-web [WIP] WASM support with grpc-web Jul 3, 2021
@shravanshetty1
Copy link
Author

Actually after manual testing works fine for normal grpc, not sure how

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

2 participants