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

feat: Streaming support #373

Merged
merged 5 commits into from
Nov 2, 2021
Merged

feat: Streaming support #373

merged 5 commits into from
Nov 2, 2021

Conversation

Jille
Copy link
Contributor

@Jille Jille commented Oct 25, 2021

I just went ahead and implemented my proposal from #372.

This should make it possible to do streaming RPCs, given that the user's Rpc implementation implements a few extra methods.

Not sure why the Reader/Writer order was changed in the generated files. I'm assuming it's because I changed some calling order, but I didn't investigate.

It still won't compile, but this at least makes the class match the
interface.
@Jille Jille changed the title Streaming support feat: Streaming support Oct 25, 2021
If you define a single streaming RPC (client, server or bidirectional
streaming), the Rpc interface will require you to also implement
clientStreaming, serverStreaming and bidirectionalStreaming. Those
methods will be passed/return Observables.

Previously, the generated file when using streaming RPCs did not
compile.
Jille added a commit to Jille/twirp-rpc-client that referenced this pull request Oct 26, 2021
Copy link
Owner

@stephenh stephenh left a comment

Choose a reason for hiding this comment

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

Hey @Jille ! Thanks for the PR! I haven't personally done a lot of streaming, so contributions to flush this out is great. Looks like maybe its just prettier failing the CI checks, but otherwise lgtm!

@Jille
Copy link
Contributor Author

Jille commented Oct 31, 2021

Thanks @stephenh!

There seems to be one other weird thing from the CI: https://github.com/stephenh/ts-proto/runs/4037980567?check_suite_focus=true - It rewrites integration/grpc-web-go-server/example.ts and integration/no-proto-package/no-proto-package.ts and eats all the indentation. I don't see how that'd be caused by my PR, is it a known issue?

@stephenh
Copy link
Owner

@Jille ah yeah, those errors can be hard to debug; there is a likely-silly syntax error, which means that the last "run prettier to get nice indentation" step can't run, and so you're seeing the raw ts-proto output. Maybe try running ./codegen.sh grpc-web-go-server locally and then opening the file in vscode/etc. to see if it will, ugly formatting aside, point you in the right direction of the syntax error.

This fixes broken generated code that somehow didn't happen for all tests but
only a few.
@Jille
Copy link
Contributor Author

Jille commented Oct 31, 2021

Thanks, that was the hint I needed :)

@stephenh
Copy link
Owner

stephenh commented Nov 2, 2021

Looks good @Jille ! Going to hit merge and should be auto-released.

@stephenh stephenh merged commit 459b94f into stephenh:main Nov 2, 2021
stephenh pushed a commit that referenced this pull request Nov 2, 2021
# [1.85.0](v1.84.0...v1.85.0) (2021-11-02)

### Features

* Streaming support ([#373](#373)) ([459b94f](459b94f))
@stephenh
Copy link
Owner

stephenh commented Nov 2, 2021

🎉 This PR is included in version 1.85.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants