kuzzleio / sdk-javascript

Kuzzle Javascript SDK. High level library including multi-protocol support, offline resiliency, realtime data and more
http://docs.kuzzle.io/sdk/js/7
Apache License 2.0
39 stars 15 forks source link

Suggestion: Event `disconnected` should be fired when .disconnect is called #298

Closed lethak closed 6 years ago

lethak commented 6 years ago

Suggested Behavior

Event disconnected should be triggered upon manually calling .disconnect() in manual connection mode.

Perhaps with a callback argument specifying is was from a manual action and not an unexpected one.

Current Behavior

Event disconnected is NOT triggered upon manually calling .disconnect()

Steps to Reproduce

const vm = {isConnected:false}
kuzzle.addListener('connected', () => {
  vm.isConnected = true
})
kuzzle.addListener('reconnected', () => {
  vm.isConnected = true
})
kuzzle.addListener('disconnected', () => {
  vm.isConnected = false
})

kuzzle.connect() //  event connected is triggered, vm.isConnected === true

// ... do some stuff ...

kuzzle.disconnect() //  event disconnected is NOT triggered, vm.isConnected === true and kuzzle.state === 'disconnected'

Context

Trying to create a Vue JS wrapper Using docker image: kuzzleio/kuzzle:1.3.1

Aschen commented 6 years ago

Hi @lethak ! Thanks for your report, we will investigate this issue shortly.

Aschen commented 6 years ago

Hi @lethak,

This behavior is intentional. The disconnected event is triggered by unexpected disconnection only, so you can implement some logic to reconnect to Kuzzle (See SDK Events). In the meantime, Kuzzle switches to offline state and all subsequent request will be queued.

In the other hand, if you are manually calling the disconnect() method, the disconnection in intentional. Kuzzle switches to disconnected state. Request won't be queued and you can't make any request to the API until an other call to connect().

We are aware that the documentation was not very clear about this, we have now improved it.

Thanks you for your feedback !

lethak commented 6 years ago

Hi,

I understood that from the documentation, but this is a suggestion to change this frankly illogical behavior.

Of course I can create layers upon layers of my own event hub in front of kuzzle, but this is supposed to be a core event imho.

Aschen commented 6 years ago

We agreed that the name disconnect() for this method may be inappropriate and can lead to confusion. We are thinking about something else for the next version, feel free to propose a different name :)

scottinet commented 6 years ago

Hi @lethak,

This is a choice we made deliberately, because the SDK instance is in an entirely different state whether the event is called after an unexpected disconnection, or after an explicit call to disconnect(). Listeners on that event would have to handle different cases, and it kinda kills the purpose of an event system.

When we discussed your issue internally, we first proposed to add a new, dedicated event. We discarded that idea because adding an event triggered only after an intentional call to a function by a SDK user didn't seem like an improvement.

That said, I believe there is room for improvement regarding our event names.