don / cordova-plugin-ble-central

Bluetooth Low Energy (BLE) Central plugin for Apache Cordova (aka PhoneGap)
Apache License 2.0
945 stars 609 forks source link

Missing docs for startNotification result. #1005

Open fudom opened 8 months ago

fudom commented 8 months ago

There is a lack of information in the startNotification docs.

It seems that the received result is an array [dataBuffer, nextSequenceNumber] (type: [ArrayBuffer, number]). There is an example with data[0]. But you don't see, why using the first index here. After digging into the code, I found that the second index is a next sequence number - whatever that is.

https://github.com/don/cordova-plugin-ble-central/blob/1727fd1d305f3265a72debe51af81d515ec40a19/src/android/SequentialCallbackContext.java#L39-L49

I'm not sure what the result is on iOS. And is sequence number required? What is the use case? But at least, this should be visible in the docs.

In the types.d.ts is only the ArrayBuffer. Is this correct? I'm confused. Update: It seems to be correct. Not sure why the [dataBuffer, nextSequenceNumber]. Tested with withPromises.startNotification.

https://github.com/don/cordova-plugin-ble-central/blob/1727fd1d305f3265a72debe51af81d515ec40a19/types.d.ts#L154-L160

Btw. avoid any type. In this case, why should callback return any? Just use void. I would also be consistent on variable naming. The convention is JS is camelCase.

peitschie commented 8 months ago

@fudom the array there gets flattened out by the callback when returning to javascript. I.e., the success callback on the javascript side is effectively a function like function success(dataResult, sequenceResult).

Having said that, it looks like I broke the sequenceResult in the callback, and agree the typing needs to be updated to reflect this.

Just as an aside, the use of any in the callback return types makes it easier to write async callbacks (which are effectively functions that return promises). Some of these are slightly legacy features, but changing it to a stricter void return types would be a breaking change for some codebases (some of the ones I maintain would definitely require changes).

In general, I work very hard to ensure no breaking changes 🙂 .

I'll clarify the types and documentation around this. Thanks for noticing!