Closed radumvlad closed 4 years ago
ping? :(
Sorry for such a late reply. I am trying to find someone willing to look into this and opinionate.
As far as I remember the lack of initial state was picked because it is cleaner to add it to the beginning of the stream than to skip the first value (as it is an assumption about the internals of the observable) – merely not having this situation needed in the examples does not mean that noone has it and there was such a situation (I do not remember details now though).
I am not sure about .unknown
vs .unsupported
either — I need to look through the code and iOS implementation but I expect it was like this for a reason? Did you encountered a situation when .unsupported
was returned when the capability was there?
Hello @dariuszseweryn !
Regarding the .unknown
vs .unsupported
: Yes. It happened few times, on several different devices that had bluetooth and we didn't know exactly why or in what case. After we did this change, it stopped happening so we figured out this was the culprit.
Regarding the initial state, a pattern that already exists in this sense is Apple's existing implementation. Creating a CBCentralManager with delegate will also call the centralManagerDidUpdateState:
method on the delegate informing the user of the initial state.
Also, I believe that although the implementation is a little bit simpler, the user might miss-use it and fall in a trap. A common behavior is to check whether the BT state is on/off and act upon that. That means using startWith
, and, as I mentioned in my first post, if the users are not careful with it, it might be flaky. Even after acknowledging in a project that we need deferred syntax for that startWith, after a couple of months we accidentally introduced the same bug of forgetting to use it.
This PR will add 1 fix and 1 improvement for BluetoothState methods:
unknown
if CBCentralManager doesn't have a state yet. This feels the right state to report in this case, since we can't say for sure that Bluetooth is unsupported.observeState()
. This will also be emitted upon subscription, using deferred syntax. The main reason behind this is that perhaps all use-cases when observing the BluetoothState you will also want to check the initial state. All the examples in the code also use this and any app that wants to present BT state will probably also need an initial value. Moreover, the examples in the code currently usestartWith
, but they don't take into consideration when the subscription happened. If the subscription happens at a later time, that startWith might have a different value, and it becomes useless. In order to fix this, the user will always need to use thedeferred
syntax to be sure that he actually has the latest bluetooth state. This feels like an improvement that we can do for the users.