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

--ts_proto_opt=returnObservable=false still returns Observable in stream methods #337

Open
cody-dot-js opened this issue Jul 23, 2021 · 3 comments

Comments

@cody-dot-js
Copy link

cody-dot-js commented Jul 23, 2021

is this by design?

also, it appears the output is incorrectly generated, or at least something that tsc hates, e.g.:

Given:

service Foo {
  rpc Sync(stream SyncReq) returns (stream StreamItem);
}

Outputs:

Sync(
  request: DeepPartial<Observable<SyncReq>>, // [1]
  metadata?: grpc.Metadata,
): Observable<StreamItem> {
  return this.rpc.invoke(
    FooSyncDesc,
    Observable<SyncReq>.fromPartial(request), // [2], this line is problematic to tsc
    metadata,
  );
}

I'm also using --ts_proto_opt=outputClientImpl=grpc-web, which gives me this definition for invoke:

invoke<T extends UnaryMethodDefinitionish>(
  methodDesc: T,
  _request: any,
  metadata: grpc.Metadata | undefined
): Observable<any> {
  // Status Response Codes (https://developers.google.com/maps-booking/reference/grpc-api/status_codes)
  const upStreamCodes = [2, 4, 8, 9, 10, 13, 14, 15]; 
  const DEFAULT_TIMEOUT_TIME: number = 3_000;
  const request = { ..._request, ...methodDesc.requestType }; // [3]
  const maybeCombinedMetadata =
  metadata && this.options.metadata
    ? new BrowserHeaders({ ...this.options?.metadata.headersMap, ...metadata?.headersMap })
    : metadata || this.options.metadata;
  return new Observable(observer => {
    const upStream = (() => {
      const client = grpc.invoke(methodDesc, {
        host: this.host,
        request,
        transport: this.options.streamingTransport || this.options.transport,
        metadata: maybeCombinedMetadata,
        debug: this.options.debug,
        onMessage: (next) => observer.next(next),
        onEnd: (code: grpc.Code, message: string) => {
          if (code === 0) {
            observer.complete();
          } else if (upStreamCodes.includes(code)) {
            setTimeout(upStream, DEFAULT_TIMEOUT_TIME);
          } else {
            observer.error(new Error(`Error ${code} ${message}`));
          }
        },
      });
      observer.add(() => client.close());
    });
    upStream();
  }).pipe(share());

Disclaimer, we don't actually use these methods on our web client (at least yet), but they cause our tsc checks to fail.
Also, I'm unfamiliar with rxjs observables, but from a cursory glance at their docs and referring to the lines I commented with // [1], // [2], and // [3], it's unclear to me that the generated Sync method is correct...

because of the object spread in // [3], it leads me to believe that:

it seems like // [2] should be something like new Observable(SyncReq.fromPartial(request)) (i know that's not how it's constructed, but still) OR just SyncReq.fromPartial(request)

either way, those two suggestions fail because request is a deep partial Observable, instead of just a SyncReq... there's no .fromPartial() method on an observable object...


ts_proto version: latest (1.82.2)
node version: lts/* (14.17.3)

@cody-dot-js
Copy link
Author

This may be a duplicate of #329

@paralin
Copy link
Collaborator

paralin commented Jul 28, 2022

A streaming response is just that, a stream of values. So, how do you expect to expose a stream of values without an Observable?

@luhuaei
Copy link
Contributor

luhuaei commented Jul 29, 2022

A streaming response is just that, a stream of values. So, how do you expect to expose a stream of values without an Observable?

Can use callback implements this. Like https://github.com/improbable-eng/grpc-web/blob/master/integration_test/ts/_proto/improbable/grpcweb/test/test_pb_service.js#L242

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

No branches or pull requests

3 participants