Introduce grpc.web.ClientReadableStreamDelegate

[Yannic]

This CL introduces grpc.web.ClientReadableStreamDelegate, a type-safe
alternative to grpc.web.ClientReadableStream.prototype.on. This is an
improvement for type-checking of clients which currently need to suppress
reportUnknownTypes.

[stanley-cheung]

Thanks for the contribution.

We had this discussion internally about this API when we started this project and we kept this API for two reasons:

• the official Node stream API has the same API: https://nodejs.org/api/stream.html#stream_api_for_stream_consumers
• gRPC Node has the same API: https://github.com/grpc/grpc-node/blob/master/packages/grpc-js-core/src/call.ts

But I do understand the desire to have more type-safety per se. Is there another library/project where these type of onData, onEnd, onError style API are being used? And perhaps, for a middle ground, can we keep both?

[stanley-cheung]

Oh, actually, now that I read this again, you are actually not changing the public on('data', ... API.

[Yannic]

Thanks for your insights.

Changing the current on('data', ... API isn't the intent of this change. Both APIs can exist at the same time (With the exception that, at least with the implementation proposed here, they cannot be used at the same time).

I have seen this delegation-pattern style in other libraries before, but I cannot think of a concrete example right now. Having on*-style functions is very common, see https://google.github.io/closure-library/api/goog.fx.Animation.html#onBegin .
Another way of achieving more type-safety would be to add individual setOn{Data,End,Error,Status} methods. Not sure if this higher API surface is desirable, though.

[stanley-cheung]

Yea looks like this PR does not alter the public API and only offers more type-checking as an implementation details, which I am OK with that.

So it looks that you may wan to do the same thing for StreamBodyClientReadableStream too?

[jonahbron]

@Yannic Maybe I'm misunderstanding what you mean by "type safe", but it sounds like you're talking about TypeScript typings. Can a similar effect not be achieved using multiple d.ts function signatures? Something like:

on(event: 'data', (chunk: R) => void);
on(event: 'error', (error: Error) => void);
on(event: 'end', () => void);

Again, pardon if I'm misunderstanding the goal here.

[Yannic]

@stanley-cheung Yeah, StreamBodyClientReadableStream will get the same method too. Just haven't done that yet to avoid doing unnecessary work. Will finish this PR soon.

@jonahbron I'm talking about the type system of google's closure compiler, which uses JSDoc-like comments to declare types. Unfortunately, what you describe isn't possible, at least not with the open-source version of closure-compiler. Haven't seen googles internal version, though.

[Yannic]

This is ready for review now.

[stanley-cheung]

npm test failed. Not sure if you can see the Kokoro test run output, but here's the relevant section:

  protoc generated code
    ✓ should exist
    ✓ should import
  grpc-web generated code (commonjs+grpcwebtext)
    ✓ should exist
    ✓ should import
    ✓ should send unary request
    ✓ should receive unary response
    ✓ should receive streaming response
    1) should receive trailing metadata
    2) should receive error
  grpc-web generated code (closure+grpcwebtext)
    ✓ should exist (5952ms)
    ✓ should import (39ms)
    ✓ should send unary request
    ✓ should receive unary response
  grpc-web plugin test, with subdirectories
    ✓ should exist
    ✓ should import
    ✓ should send unary request
  grpc-web plugin test, with multiple input files
    ✓ should exist
    ✓ should import
  16 passing (22s)

I can reproduce it locally.

$ cd packages/grpc-web
$ npm install
$ npm test

Looks like this self.onStatusCallback_ occurance has not been properly replaced?
https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpcwebclientreadablestream.js#L166

Maybe there's some other too. Please look into that. Thanks!

[stanley-cheung]

Is this still needed?

[stanley-cheung]

Variable should be renamed?

[Yannic]

Looking at this again, I'm starting to get some mixed feelings whether we should really add this because it may not be the best idea to have two seperate APIs on one class doing the exact same thing. I think it would be better to have a compiler option choosing between two seperate implementations.

Leaving this open for now, I will post some more information about this plan soon (Hopefully also helping what's asked in #260).