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

Add onConnectionStateChanged on ClientConnection #563

Closed
wants to merge 1 commit into from

Conversation

Ludonope
Copy link

This PR adds a callback on ClientConnection to get updates when the ConnectionState changes.

Usage:

final conn = await clientChannel.getConnection();
conn.onConnectionStateChanged = (ConnectionState prevState, ConnectionState state) {
  // ... do whatever.
}

This is a first solution for issue #428. It's not perfect yet and could be improved upon, I'm open to critics/suggestions.

👀 Why is it important?

A modern app (particularly GUI) should be able to provide feedback to user related to the state of the connection. We don't want the user to wait until they interact to tell them that "oh actually the connection is down", we need to tell them as soon as possible.

🛠 Possible improvements

  1. Maybe we could set that callback as a parameter of the ClientChannel ? That way we wouldn't have to force the creation of the connection just to set it.

  2. Is the previous state necessary ? I put it there as I feel like it's important, going from idle to connected is not the same as going from transientFailure to connected. But we could also leave that to the user to store the previous state 🤔

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 22, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: Ludonope / name: Ludovic Petrenko (16d48ae)

@Cobinja
Copy link
Contributor

Cobinja commented Jun 26, 2022

If this is merged then do not merge #565.

@mraleph
Copy link
Member

mraleph commented Jun 27, 2022

Thanks for contribution @Ludonope!

I think stream based API proposed by @Cobinja is more in line with Dart style (it also allows for multiple listeners out of the box, etc). Let's try to land #565 instead of this one.

@mraleph mraleph closed this Jun 27, 2022
@Ludonope
Copy link
Author

Yeah makes more sense with stream!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants