dpa99c / cordova-diagnostic-plugin

Cordova/Phonegap plugin to manage device settings
540 stars 361 forks source link

Proposition regarding change handlers in the ionic native wrapper to return an Observable instead of having a synced callback #329

Closed somq closed 6 years ago

somq commented 6 years ago

I'm submitting a ...

Feature request

Current behavior:

Currently the ionic native wrapper let us pass a function to be called back for any change handlers.

Expected behavior:

For convenient reasons I think it would be better to have these handlers return an observable instead.

Steps to reproduce:

Current usage makes impossible to directly add registerBluetoothStateChangeHandler to an rx stream.

this.BTStateChangeHandler = new Subject()

this.diagnosticPlugin.registerBluetoothStateChangeHandler(BTState => {
  this.BTStateChangeHandler.next(BTState)
})
...

Cordova implementation is simple (source):

  /**
   * Registers a function to be called when a change in Bluetooth state occurs.
   * @param {Function} handler
   */
  @Cordova({ platforms: ['Android', 'iOS'], sync: true })
registerBluetoothStateChangeHandler(handler: Function): void {}

would become

  /**
   * Registers a function to be called when a change in Bluetooth state occurs.
   * @returns {Observable<any>}
   */
  @Cordova({ "callbackOrder": "reverse", "observable": true, "platforms": ["Android", "iOS"] })
registerBluetoothStateChangeHandler(): Observable<any>;

I listed these methods:

These handlers are exactly what Observables are meant for, a stream where it can be nexted a new value anytime.

Let me know what you think about this idea and if there is some things that has to be taken into consideration such as managing the breaking change, or maybe adding other methods aswell for instance.

I can also prepare the pull request if needed.

dpa99c commented 6 years ago

This repo contains the Cordova plugin code consisting of the native code and pure Javascript plugin API.

The Ionic Native typescript wrapper that wraps this plugin belongs to the ionic-native repo and therefore any suggestions/improvements should be opened on its issues page, not this plugin's.

somq commented 6 years ago

Of course it belongs to ionic-native. In fact I could even had directly prepare a PR for ionic-native repo but I wanted to have your opinion before doing anything that would break the existing. I'm forwarding this issue to the ionic-native repo. If you want to, let me know what you think of this idea here then please.

Thanks for your time.