Update library to use JS Promises instead of callbacks

[Steague]

Now methods can be chained together.

This PR contains breaking changes. All callback logic has been removed and replaced with Promise logic.

Unit tests have been updated.

[todbot]

Hi @Steague,
What's the minimum version of Node that is required to use Promises? I'd like to see node-blink1 not exclude anyone with slightly older versions of Node. (because they're locked into some enterprise upgrade scheme)

Alternatively, is it possible to fallback to callbacks if Promises aren't supported?

(Similar question for ES6 vs ES5 syntax)

[Steague]

@todbot

Node.js added native promise support since version 0.11.13. (Which was released in May, 2014.)
Node.js added native ES6 support since version 4.3.2. (Which was released in March, 2016.)

I would consider neither 0.11 nor 4.3 to be slightly older and are, in fact, significantly old.

Since the parameters of the functions have been updated to accept an object instead of many parameters, adding a callback property to that object would not be difficult but I see it as redundant ad backwards-compatibility is already not an option with this PR.

[sandeepmistry]

@Steague thanks for submitting!

I agree with @todbot, I'd prefer not to have breaking changes and force users into using promises.

Some proposals to go forward:

1. Have a backwards compatible callback mode as discussed above

2. Have something like var Blink1 = require('node-blink1).Promise where users can opt-into promises

3. Have another npm module for Blink1 with promises.

[Steague]

@sandeepmistry & @todbot I understand where you guys are coming from. I'll make my own API in these cases.

By the way, Node 4.x is only going to be supported for the next 3.5 months. https://github.com/nodejs/Release

I would recommend determining backwards compatibility with the current code before moving to a 1.0 release.

[todbot]

I think there is a way to support both callbacks and Promises in a single API. Something like:

class Blink1 { 
  // ...  
  _readResponse(callback) {
    if( !_isValidCallback(callback) ) { 
      return new Promise(resolve => {
        resolve(this.hidDevice.getFeatureReport(REPORT_ID, REPORT_LENGTH));
      });
    } // else callback
    callback.apply(this, [this.hidDevice.getFeatureReport(REPORT_ID, REPORT_LENGTH)]);
  }
  // ...
}

Does this seem reasonable? I admit to not having much experience with Promises. I'd like to see them supported (and ES6. I was surprised to hear Node had it back to v4)

The main reason why I was concerned was for the embedded Linux users stuck on Node v0.10 or v0.12. But cursory research shows these systems are getting Node v4.x and v8.x. (I can't believe Raspberry Pi's Raspbian still ship w/ Node v0.10.29) In any case, Nodejs.org is making it easier to get Node from them directly.

So I say let's start moving towards Promises but I'd like to keep a callback API if the above proposal isn't dumb. (I cribbed it from https://medium.freecodecamp.org/callbacks-and-promises-living-together-in-api-harmony-7ed26204538b )

[sandeepmistry]

@todbot the proposal sounds good to me, pull requests welcome for it :)

Another option could be to always make the promise, and add a .then(callback) if user provides a callback.

[Steague]

@todbot && @sandeepmistry I have added callbacks back to the code base. They are called automatically just before the promises are returned from the various class methods. The README has been updated to reflect these changes.

Additionally, I split the code up a bit to separate the validators, helpers, and base hardware API from the main methods being used.

I also added the rgbcolor package to the codebase as a supported color parameter. It is explained in the README and is not required. This allows users to specify colors like "red" instead of needing to know the RGB values.

[todbot]

Hi @Steague, this is great, thanks! I'll do some testing with it now.

[todbot]

Hmm, getting some errors with Node v4.8.2 LTS and Node v5.12.0. SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode is there a way to enable strict mode for just blink1.js?

$ nvm use v4
$ cd node-blink1
$ npm install
$ node blink1.js
/Users/tod/projects/node/node-blink1/blink1.js:4
class Blink1 extends Blink1_Validators {
^^^^^
SyntaxError: Block-scoped declarations (let, const, function, class) not yet supported outside strict mode
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:140:18)
    at node.js:1043:3

Adding "use strict"; doesn't do the trick universally either. Adding it to blink1.js gives:

 node blink1.js
/Users/tod/projects/node/node-blink1/blink1.js:12
    version(callback = () => {}) {
                     ^

SyntaxError: Unexpected token =
    at exports.runInThisContext (vm.js:53:16)
    at Module._compile (module.js:373:25)
    at Object.Module._extensions..js (module.js:416:10)
    at Module.load (module.js:343:32)
    at Function.Module._load (module.js:300:12)
    at Function.Module.runMain (module.js:441:10)
    at startup (node.js:140:18)
    at node.js:1043:3

[todbot]

It does work with Node v6.0.0 (released Apr 2016) and Node v6.11.2 LTS. I guess I'm okay with that. We should definitely put in the README that this upcoming version is Node v6 and above and older Node users should use the soon-to-be previous version.

[sandeepmistry]

So this will still have breaking changes to the API right? Some parameters are an object now instead of multiple parameters.

@todbot opinions on this? It's going to be a breaking change, so will require a major rev update for semver, if Node 6 is the minimum version as well.

[fushi]

Is this still being considered? Promises are a core feature of the language now.