bug: stream should force observable

[toonvanstrijp]

Heey there, I'm quite new to GRPC and protobuffers. Great library btw! I'm using the nestjs feature and it's working great so far!

However I think when defining a stream parameter or return type it should be forced to be of type Observable right? Cause a Promise can't support a stream of data.

The proto file I have:

syntax = "proto3";

import "google/protobuf/empty.proto";

package statistic;

service StatisticService {
  rpc FindOne (StatisticById) returns (Statistic);
  rpc FindMany (stream StatisticById) returns (stream Statistic);
  rpc CreateEmptyRow (EmptyStatisticRow) returns (google.protobuf.Empty);
}

message EmptyStatisticRow {
  int32 id = 1;
  string field = 2;
}

message StatisticById {
  int32 id = 1;
  string field = 2;
}

message Statistic {
  int32 entityId = 1;
  string sales = 2;
  int32 revenue = 3;
  int32 revenueCash = 4;
  int32 revenueDigital = 5;
}

As you can see the FindMany call takes a stream as input and returns a stream.

Right now ts-proto outputs this function as followed:

  findMany(request: StatisticById, metadata?: Metadata): Promise<Statistic>;

You can force the return type by providing this option --ts_proto_opt=returnObservable=true but this only changed the return type not the parameter type. Also if you set this option to false and the rpc call returns a stream it should be forced to the Observable type right?

I'm currently looking at the nestjs documentation: https://docs.nestjs.com/microservices/grpc#subject-strategy

And as you can see they wrap the input in an Observable and the return type also.

fix

I already implemented a fix and can create a pull request for this. But my main question is if we should do this if the nestJs option is true or not.

debugged@dd5a808

[stephenh]

Hi @toonvanstrijp yeah, good call out. I've not actually used streams in protobuf on node; I did awhile ago in some protobuf on JVM but kinda forgot about streaming for node so far. :-)

I think your initial fix is a good first step...

Since you're an NestJS user, can you extend the nestjs-simple integration test that @iangregsondev just added:

https://github.com/stephenh/ts-proto/tree/master/integration/nestjs-simple

To have an additional .proto message/method with stream in it?

And then run the update-bins.sh and codegen.sh to update the nestjs-simple output to ensure it works the way you expect, i.e. most things will still be Promise but the stream will be Observable.

If we have that, I think that's a good enough to accept. I.e. if it works for you and NestJS, I'm fine just having a known-issue that streaming is still unsupported for non-NestJS use cases.

Two things:

1. fwiw this example:

@GrpcStreamCall()
bidiHello(requestStream: any) {
  requestStream.on('data', message => {
    console.log(message);
    requestStream.write({
      reply: 'Hello, world!'
    });
  });
}

Makes me think that, if you use streams (maybe as input parameters?), the service interface is no longer 100% the same for client and server, b/c the server-side method accepts a grpc.ServerReadableStream and the client-side method accepts a ...ReplaySubject/subject. (FYI @iangregsondev ).

2. I would love to have the nestjs-simple (or similar) integration test have a unit test that actual standups a NestJS server on local port whatever, instantiates a NestJS client, and watches the client be able to make calls to the server (again FYI @iangregsondev , I think this would be a great "tutorial" for NestJS users + a really great regression test to include in ts-proto to make sure we know "for sure" things owkr).

At first this test could be just for regular synchronous calls, but then it'd be great especially for these streaming methods, given the APIs can be fairly unintuitive at first.

But I'm fine punting on this #2 ask if you don't want to tackle it in the 1st "return

[iangregsondev]

Great catch @toonvanstrijp , I must admit I don't use the streaming option so I kind of never investigated it - but your fix looks good.

I kind of always set everything to Observable :-) Although that's a mistake on my part as others might not want this and then the stream part would fail.

@stephenh I believe the client/server API interface can be the same, in-fact that's the way I would use it. Returns an observable and accepts an observable - I think these both work on client/server.

The thing is that the implementation supports way more stuff :-)

Call stream handler#
When the method return value is defined as stream, the @GrpcStreamCall() decorator provides the function parameter as grpc.ServerDuplexStream, which supports standard methods like .on('data', callback), .write(message) or .cancel().

So it kind of supports a call back on the client according to the nestjs docs.

I must admit, I don't use it, so I am not the best person to comment.

@toonvanstrijp Can you confirm.

Maybe this callback is a kind of edge cases for most systems and not needed or used minimal? Not sure, but if this is the case - then maybe we can say - if you require a custom implementation that doesn't use the generated interface and create it manually - if it's only for 1 or 2 methods.

Again - I am not sure the right approach here.

Cheers!

[toonvanstrijp]

@stephenh @iangregsondev
Woah you guys are fast! Thanks for your replies.

First of all, yes I'll add a test for this inside nestjs-simple.

2. Then there is the @GrpcStreamCall() I think this one does the same as returning / accepting an observable. I think nestjs add the Observable support cause it seems a bit more elegant in the nestjs framework.

As I said I'm quite new to this but we could add an option inside the proto file so you can tell if the rpc call needs to be formatted confirm the Observable pattern or in the GrpcStreamCall way?

extend google.protobuf.MethodOptions {
  optional bool native_grpc_stream_call = 50000;
}

I'm not sure if this is the right way to make this possible, so if you guys have other ideas let me know ;)

[toonvanstrijp]

@stephenh I'm trying to run the update-bins.sh but I'm missing this tool: protoc-gen-dump. How do I set up the development environment?

[iangregsondev]

@stephenh I'm trying to run the update-bins.sh but I'm missing this tool: protoc-gen-dump. How do I set up the development environment?

Did you follow this in the readme ? I also had some initial problems but i think it was fixed following this.

After running yarn install (which will fail in yarn test on the first time), run ./pbjs.sh to create the bootstrap types, and ./integration/pbjs.sh to create the integration test types. These pbjs-generated files are not currently checked in.

After this the tests should pass.

After making changes to ts-proto, you can run cd integration and ./codegen.sh to re-generate the test case *.ts output files that are in each integration// directory.

[toonvanstrijp]

@iangregsondev ow haha I was running it from the wrong directory. I was doing this: ./integration/update-bins.sh but I need the cd into integration and then run ./update-bins.sh

[toonvanstrijp]

@stephenh @iangregsondev

Alrighty I'm making some progress and found out that there is a difference between the client / server implementation of nestjs.

If we take the following proto:

service HeroService {
    rpc FindOneHero (HeroById) returns (Hero) {}
    rpc FindOneVillain (VillainById) returns (Villain) {}
    rpc FindManyVillain (stream VillainById) returns (stream Villain) {}
}

And we generate the ts file:

export interface HeroService {

  findOneHero(request: HeroById): Promise<Hero>;

  findOneVillain(request: VillainById): Promise<Villain>;

  findManyVillain(request: Observable<VillainById>): Observable<Villain>;

}

The interface is correct for server implementation. Hover for nestjs it doesn't matter if you return an Observable or Promise (only when using streams). When nestjs receives a Promise it'll transform this in the client to an Observable.

So the server typescript could look like this:

export interface HeroServiceServer {

  findOneHero(request: HeroById): Promise<Hero> | Observable<Hero> | Hero;

  findOneVillain(request: VillainById): Promise<Villain> | Observable<Villain> | Villain;

  findManyVillain(request: Observable<VillainById>): Observable<Villain>;

}

This way it matches the nestjs format, since nestjs is taking care of the transformation between different types.

Then the client typescript file should look like this:

export interface HeroServiceClient {

  findOneHero(request: HeroById): Observable<Hero>;

  findOneVillain(request: VillainById): Observable<Villain>;

  findManyVillain(request: Observable<VillainById>): Observable<Villain>;

}

I think when nestJs=true we need to create those two different server and client interfaces so that it matches the nestjs way of how to integrate this.

What is your opinion on this?

[toonvanstrijp]

@stephenh @iangregsondev
also I added some tests!
Take a look! debugged@7213e8b

As you can see right now I'm doing a ugly type casting here: debugged@7213e8b#diff-f54c909de34198d5f4758846c62e72efR41

But we can solve this by implementing the solution I provided above.

[iangregsondev]

The interface is correct for server implementation. Hover for nestjs it doesn't matter if you return an Observable or Promise (only when using streams). When nestjs receives a Promise it'll transform this in the client to an Observable.

Yes true, nestjs will convert this to standard object as its going over the wire. There is no such thing as an observable when going over the wire.

The main thing it gives you is how to deal with your code inside the service methods, dealing with ASYNC and AWAIT, pure promises, observables or simple objects.

So the server typescript could look like this:

export interface HeroServiceServer {

  findOneHero(request: HeroById): Promise<Hero> | Observable<Hero> | Hero;

  findOneVillain(request: VillainById): Promise<Villain> | Observable<Villain> | Villain;

  findManyVillain(request: Observable<VillainById>): Observable<Villain>;

}

ermm, I never thought of returning union types. I kind of like it :-) Me as a developer can decide to return what I want - but this means it is slightly less typed ah ah. I mean it isn't specifically typed to one return type but to 3 (union) - but I think this works.

Then the client typescript file should look like this:

export interface HeroServiceClient {

  findOneHero(request: HeroById): Observable<Hero>;

  findOneVillain(request: VillainById): Observable<Villain>;

  findManyVillain(request: Observable<VillainById>): Observable<Villain>;

}

I must admit I have never tested it, but are you saying that the client can only receive Observables? Not sure here, the client I think can at least receive standard objects or observables - ?? Maybe I am wrong.

I don't really tend to use Promises anyway, but it kind of make sense I think as the information isn't sent over the wire until the promise completes on the server.

I think when nestJs=true we need to create those two different server and client interfaces so that it matches the nestjs way of how to integrate this.

What is your opinion on this?

I think it would work but implementing 2 x interfaces, 1 for client and 1 for server!

I like it, what does @stephenh think?

[toonvanstrijp]

ermm, I never thought of returning union types. I kind of like it :-) Me as a developer can decide to return what I want - but this means it is slightly less typed ah ah. I mean it isn't specifically typed to one return type but to 3 (union) - but I think this works.

I think we have to do a 3 (union) type here cause that we model nestjs correctly. If we only accept a promise or observable we're limiting the developer.

I must admit I have never tested it, but are you saying that the client can only receive Observables? Not sure here, the client I think can at least receive standard objects or observables - ?? Maybe I am wrong.

Yes the way nestjs builds the client from the .proto file it always returns the response in an Observable. So it doesn't matter if the data was sent via observable or promise. But the client wraps the response in an observable and then returns this. I've tested this.

update

1. I did some more refactoring and I think it's very close for a release. I added an nestjs section to the readme file: https://github.com/debugged-software/ts-proto#nestjs
2. It generated two interfaces, 1 for the client and 1 for the server. (only when nestjs=true)
3. I also added a naming convention for the interfaces in nestjs.
4. I created two dedicated functions for creating the server interface and the client interface. This way we can easily change the way how we build the interfaces for nestjs. (generateNestjsService and generateNestjsServiceClient).

@stephenh @iangregsondev
Please take a look at the readme and let me know if there are some changes needed for a release. https://github.com/debugged-software/ts-proto#nestjs

[toonvanstrijp]

update 2

1. when nestjs=true it also generates an class decorator for example HeroServiceControllerMethods (based on the name of the service). Normally in nestjs you would need to add @GrpcMethod('SERVICE_NAME') or @GrpcStreamMethod('SERVICE_NAME'). This however would fail in runtime when you accidentally change your service name. But because we generate this class decorator for you, you won't have this risk anymore. The only thing you need to do is provide this decorator on the controller class. The decorator will take care of applying the @GrpcMethod and @GrpcStreamMethod

@Controller('hero')
// Generated decorator that applies all the @GrpcMethod and @GrpcStreamMethod to the right methods
@HeroServiceControllerMethods()
export class HeroController implements HeroServiceController {
...
}

2. based on the .proto we'll generate a const for example HERO_PACKAGE_NAME and HERO_SERVICE_NAME this way your code breaks if you change your package or service name. (It's safer to have compiler errors than runtime errors)

[stephenh]

I agree with pretty much everything in #60 :-) The HeroServiceControllerMethods in particular is a great ergonomics trick. Next time I want to play with grpc streaming, I will be using this nestjs setup. :-)

[iangregsondev]

Great work @toonvanstrijp

Somebody reported an issue to me last night actually, I don't know if yours fixes this or not.

Here reported this in the nestjs discord channel

i see the --ts_proto_opt=nestJs=true and i used it. beautiful.
functions are lowercased. however, empty still appears for responses that should be void.
am I missing an option for this?

I still need to confirm it but I must admit I think I overlooked that :-(

If you want I can take a look after you have merged? Or you can - let me know either way.

[toonvanstrijp]

@iangregsondev It didn't but I've implemented a check for this and now it expects a type void when an Empty return is set for the response.

[iangregsondev]

nice @toonvanstrijp , yep i knew I overlooked that. Thanks!

[stephenh]

Released in v1.20.0. Thanks @toonvanstrijp