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

Make client status public #29

Closed
wants to merge 1 commit into from
Closed

Conversation

danqing
Copy link
Contributor

@danqing danqing commented Mar 9, 2020

Closes #27. This allows users to know the current status of the client.

@danqing
Copy link
Contributor Author

danqing commented Mar 9, 2020

(Also removed some trailing whitespaces as my editor automatically does so on save)

@FZambia
Copy link
Member

FZambia commented Mar 9, 2020

@danqing hello!

Thanks for pr, I have some concerns about this.

First, could you describe how you are going to use this? This method can be pretty racy - the connection can be in connected status at the moment of check but be closed when you are performing actual operation after status check. Though it's pretty the same for current situation so you always need to handle errors.

If your use case is valid then maybe we can introduce a client method client.connected() which returns bool value to not expose statuses to public access (I'd like to leave statuses internal detail).

@danqing
Copy link
Contributor Author

danqing commented Mar 9, 2020

What I want to achieve is basically what all chat apps have: let the user know when the server is disconnected. For example, if you use FB messenger, you will see a "reconnecting" banner coming up in this case.

I want to do the same: when there is connection issue, show a yellow "reconnecting" banner or a red "no connection" banner, and dismiss the banner when connection resumes.

So I need to:

  1. Know the connection state when the view controller (chat view) first launches
  2. Be aware of state changes

@danqing
Copy link
Contributor Author

danqing commented Mar 9, 2020

Btw, this is related to #16 that I opened a while ago. It's useful to know initial connection error too.

@danqing
Copy link
Contributor Author

danqing commented Mar 9, 2020

So in summary, I'd like to know at any given time the status of the client:

  • whether it's new, connected, disconnected (for good) or reconnecting
  • both be able to check and get notified of such state

I also want to understand better potential errors for subscription: if I have the permission and connection is successful, does that guarantee that subscription will succeed? If not, what's an example of failed subscription when connection itself is successful?

Thanks!

@FZambia
Copy link
Member

FZambia commented Mar 10, 2020

To implement what you need you have to track status changes with connect, disconnect events anyway. Disconnect context has reconnect field which will tell you about whether client will reconnect or not. I don't understand how having status field will make your life much easier.

@danqing
Copy link
Contributor Author

danqing commented Mar 10, 2020

Well, why should I (i.e. the user) track status changes instead of the library itself? I'd say it's a common thing to do, so wouldn't it be better if the library already does this out of the box?

@FZambia
Copy link
Member

FZambia commented Mar 10, 2020

Please post a code example on how you are planning to use public status without using connect and disconnect events. Maybe I am missing sth.

@danqing
Copy link
Contributor Author

danqing commented Mar 10, 2020

As I mentioned, I'd like to show a banner on viewDidAppear if status is not connected. To do so, I'd like to be able to check the status directly.

func viewDidAppear(_ animated: Bool) {
  super.viewDidAppear(animated)
  if centrifuge.status == .disconnected {
    // show banner
  }
}

Of course I can track the status myself, but it'd be better if centrifuge already does this.

@FZambia
Copy link
Member

FZambia commented Mar 10, 2020

The reason why I don't want to make status public at this moment is because now it differs through client implementations. At moment internal statuses are:

  1. In Swift client: NEW, CONNECTED, DISCONNECTED
  2. In Java client: NEW, CONNECTED, DISCONNECTED
  3. In Dart client: CONNECTED, DISCONNECTED, CONNECTING
  4. In Go client: DISCONNECTED, CONNECTING, CONNECTED, RECONNECTING, CLOSED
  5. In Javascript: disconnected, connected, connecting

While the public API is pretty much the same at this moment. I'd like to keep all clients consistent, so if you write code in one client it could be easily translated to another.

So if we make this part public we should carefully think on what other libraries do and make experience consistent. This is pretty big work so I prefer to avoid this at this moment until this is really necessary. Every change makes project maintenance harder, hope you can understand this.

@danqing
Copy link
Contributor Author

danqing commented Mar 12, 2020

Fair enough.

@danqing danqing closed this Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Access status of client?
2 participants