Expose onConnectionStateChanged for channels #565
Conversation
This offers a Stream<ConnectionState> on channels. Only for Http2Channel, not for web usage.
|
|
lib/src/client/http2_channel.dart
Outdated
| @@ -31,27 +33,89 @@ class ClientChannel extends ClientChannelBase { | |||
| final Object host; | |||
| final int port; | |||
| final ChannelOptions options; | |||
| final StreamController<ConnectionState> connectionStateStreamController = | |||
There was a problem hiding this comment.
Can this live in ClientChannelBase instead? Then you don't need any of the duplicated code below.
You would need to add a member to ClientChannel though to expose state changes in some form.
There was a problem hiding this comment.
Done in the new commit
There was a problem hiding this comment.
My suggestion was to move all of the code to the base class (this includes code from terminate and shutdown as well as attaching status change callback to the connection object).
There was a problem hiding this comment.
Ah, ok, I didn't catch that from the previous comments. I'll do it it tonight after work at my dayjob.
There was a problem hiding this comment.
I don't think moving everything to ClientChannelBase is the best way to go. The callback to the connection object has to stay because it's specific to http2 compared to web channels which don't have the ability to track state changes in the first place.
I'm also removing the stream getter override in the web channel, so that devs don't have to catch any exception, the stream will just do nothing (that noop is documented).
- improve documentation wording - use whenComplete instead of then when closing the stream sink
- changed version to 3.1.0-dev - Added changelog entry - moved definition of onStateChanged to ClientConnection as a setter
|
Thanks @Cobinja. I think this is good to go. I have refactored things a bit more (you introduced the field I asked for - but did not actually make use of it to share repeating code). You can take a look at the final version. |
|
My thought process was "Web channels might need a completely different implementation, so just provide the common API". I'm fine with your changes. |
|
Hi, Just a ping on this PR. What's blocking this from being merged? |
|
This was a breaking change - there are internal implementations of |
This offers a Stream on channels. Only for Http2Channel, not for web usage.
Usage:
clientChannel.onConnectionStateChanged.listen(...)
This is an alternative to #563. Only merge one of these.