ts-proto
ts-proto copied to clipboard
stephenh
Support Observables in *ClientImpl classes
Hey, I've added support for Observables to ClientImpl classes. Before this change clients weren't correctly implementing service interfaces. One thing is to be fixed though - some integration tests are failing now due to lack of Observable import statement. It should be an easy fix but I'm not super familiar with the ts-poet lib that does all the code generation so it'd be great if @stephenh could look at it.
@pittkost cool, thanks for the PR! A few questions; I don't see a change to main.ts, did that not get included?
Also, what protocol/framework are you working with? I.e. grpc / twirp / nestjs? The clientimpl classes were originally written for twirp, so just sanity checking if that was your intent, or if they are also somewhat magically working for non-twirp use cases that I wasn't aware of.
Thanks!
@stephenh Sorry, I guess I committed from within integration directory hence didn't add main.ts 😆. I've added it now and also fixed that issue I mentioned before. As for my case - we use ClientImpl for web-grpc cause it provides as with much nicer syntax than what Google's and Improbable's grpc-web libs have to offer.
@pittkost that's amazing to hear you're using ts-proto with grpc-web. I haven't personally needed to do grpc-web stuff and so wasn't sure if/how it'd work. I'm pleasantly pretty surprised that it does.
Fwiw doesn't have to be this PR, but it'd be great to have something like this:
https://github.com/stephenh/ts-proto/blob/master/integration/nestjs-simple/nestjs-simple-test.ts
For grpc-web, which shows "boot up a test server, a test client, and have them talk to each other".
I'll try and get your PR reviewed today. Thanks!
I've just come across this issue when generating *ClientImp. @pittkost did you end up using this library based on your fork, or give up in the end?
@snikch actually I forked the whole project and made some more changes, but as far as I remember this fix was sufficient to to solve the issue.