Last update (v1.2.0) broke promise based unary call

[Fl0pZz]

#746

And this file does not have any test for thus:
https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/test/generated_code_test.js

[stanley-cheung]

Can you give us a bit more information on how it broke? Which mode are you using? Can you share some code snippet?

#746 was included in the 1.1.0 release. Did it also break there?

There are some tests for the Promise-based unary calls here: https://github.com/grpc/grpc-web/blob/master/packages/grpc-web/test/plugin_test.js#L241. Not a lot but should cover some basic cases. Perhaps you are using a different mode?

[smnbbrv]

We have I guess a connected issue at https://github.com/ngx-grpc/ngx-grpc

      const stream = this.client.rpcCall(
        this.settings.host + path,
        req,
        metadata || {},
        new AbstractClientBase.MethodInfo(
          resclss,
          (request: Q) => reqclss.toBinary(request),
          resclss.fromBinary
        ),
        () => null
      );

      stream.on('data', data => obs.next(new GrpcDataEvent(data))); // <= the data event is not triggered anymore

The problem occurs only for unary calls. Server streaming works fine.

@stanley-cheung shoud the data event be emitted on unary calls?

P.s.: v1.1.0 still emits data event.

[stanley-cheung]

@smnbbrv This is intentional. In the 1.2.0 release, in work like #847 and #858, we tried to fix the behavior of these callbacks. In your case, you should be passing in an (err, response) => ... callback in your last argument, instead of () => null as now. Unary calls fundamentally should have different semantics than streaming calls. So in the last release, we had disabled the .on('data') and .on('error') callbacks for unary calls. You should be receiving those via the main (err, response) => ... callback. Consider the case from the other side, had someone uses the main callback (err, response) => ..., and the .on('data') callback, both will fire and that's confusing. (And same for the err and .on('error') pair.)

This is also an effort to align the behavior to be consistent with grpc-node. For unary calls, you should not be receiving response via the .on('data') callback either. I probably should have called this out in the release notes as "breaking change", but this is more like a bug fixing an old incorrect behavior. We have added a unit test specifically to call out this behavior.

In #858, there's a table summarizing whether each callback will be triggered in different scenarios. Please check that out.

Also, I noticed that you are not using the generated code surface and are calling directly into the generic layer. There you might want to be careful as that's not the general supported API surface and we may be changing this layer in the future. In fact, I noticed that you are still using the MethodInfo class which will be deprecated soon and be replaced by the MethodDescriptor class. The current generated code is already using the MethodDescriptor class. Please check that out.

[smnbbrv]

@stanley-cheung thank you very much for the detailed answer!

You should be receiving those via the main (err, response) => ... callback. Consider the case from the other side, had someone uses the main callback (err, response) => ..., and the .on('data') callback, both will fire and that's confusing. (And same for the err and .on('error') pair.)

This is exactly the error I had before, and this was the reason why I chose the event "way", to be more similar to the streaming syntax. However, now I simply switch to the proper way of handling the unary response.

Also, I noticed that you are not using the generated code surface and are calling directly into the generic layer. There you might want to be careful as that's not the general supported API surface and we may be changing this layer in the future.

I know that using the underlying methods is dangerous, however, there was no other option (at the implementation time) to get it working. Also, the risk is accepted and I tried to prepare to those breaking changes by generalising the client implementation as much as it is possible; so, ideally it should survive breaking changes.

I will learn the changes and will come back in case of problems :) Thank you again for your time and help.

[smnbbrv]

@stanley-cheung whoops, the TypeScript definitions do not have the MethodDescriptor exported and I can remember that I already have seen the MethodDescriptor in the JavaScript generated files; however TypeScript used the MethodInfo so did I, because the latter was the only one that had typings.

With bypassing the typings

declare var require;

console.log(require('grpc-web').MethodDescriptor);

I can read it in TypeScript.

Looks like grpc-web's TypeScript mode still uses MethodInfo as well, as long as MethodDescriptor is not available there. If yes, it deserves a separate issue...

[stanley-cheung]

Ah I see. Yea then that's a separate issue we need to fix then. Thanks for pointing that out.

[smnbbrv]

@stanley-cheung should I create the issue?

There is another notable difference (maybe actually it was the same before, but with events it worked differently). The event with status code OK comes before the unary callback is called, while all streaming callbacks land before the OK-code event.

This is the timeline for unary request:

And this is the timeline for server stream:

Is this intended? To my taste it is a little bit weird. Do I miss something?

[stanley-cheung]

In the streaming case, that will be the case. The status callback should come after the data callbacks.

For unary calls, there is no guarantee. You should not rely on the sequence of those events.

[stanley-cheung]

@Fl0pZz just want to check in to see if you still have issue with the 1.2.0 release? The other issue @smnbbrv brought up was intended behavior - if you have the same issue with callbacks, then I can close this issue. If you have a separate issue with the 1.2.0 release I'd like to know more about it. Thanks!

[Fl0pZz]

@stanley-cheung ok, close this issue. Thanks!

[stanley-cheung]

Thanks!