Missing cleanup prototype functions

[shanshanzhu]

When we register a listener to 'data', 'error' events, the source code is basically overriding existing methods. There is no clean ways to unregister the listener. We'd expect a method such as off, removeListener. Even better, we can useaddListener to replace current implementation of on.

https://github.com/grpc/grpc-web/blob/master/javascript/net/grpc/web/grpcwebclientreadablestream.js#L211-L224

To clean up, in current version we have to do this:
stream.on('data', () => {})

[stanley-cheung]

That's a fair point. Looks like gRPC Node's implementation uses EventEmitters. The .on() API can attach multiple listeners when called multiple times. And the returned stream object implemented the EventEmitters interface, which gives a removeListener method. Will definitely look into this.

[MihailsKuzmins]

Now there are two cleanup methods, i.e. cancel and removeListener. So I'm wondering if it's enough just to call stream.cancel on the end event, or should I remove all listeners explicitly with removeListener?
Or maybe cancel is not a cleanup method at all?

const stream = client.sayManyHellos(req, {});
stream.on("end", () => {
	stream.cancel()
})

Or more explicitly

const stream = client.sayManyHellos(req, {});
const onEnd = () => {
	stream.removeListener("end", onEnd)
	stream.cancel()
}

stream.on("end", onEnd)

[sampajano]

@MihailsKuzmins Thanks for the question!

As far as i understand, cancel() and removeListener() serves slightly different purposes (see the docs).

I haven't tried myself, but i do expect a few last callbacks (like end callbacks being invoked as in your example) after cancel() is called, unless you removes the listener also in your example.

Hope that makes sense :)

[MihailsKuzmins]

@sampajano thank you for explaining.

So as I understood it's better to remove the end callback rather than just relying on cancel(). I will adjust my code :)