Add support for Promises #458

recrsn · 2016-11-27T15:42:28Z

This pull request implements a Promise based interface to bcrypt library. A Promise is returned whenever there is no callback function specified. We have taken care not to break any existing functionality or behavior. Tests for the new interface have also been added.

mention-bot · 2016-11-27T15:42:29Z

@Agathver, thanks for your PR! By analyzing the history of the files in this pull request, we identified @defunctzombie, @shovon and @charmander to be potential reviewers.

recrsn · 2016-11-27T15:53:37Z

This undoubtedly fails on node <= 4.4 since Promises are not available

defunctzombie · 2016-11-27T16:36:12Z

bcrypt.js

@@ -36,6 +36,10 @@ module.exports.genSalt = function(rounds, ignore, cb) {
 cb = arguments[1];
 }

+ if(!cb) {


style nitpick: space after if (basically, look at the rest of the file and try to follow that style for any new code you add.

defunctzombie · 2016-11-27T16:36:54Z

bcrypt.js

+/// @param {f} function to be encapsulated
+/// @param {args} Array like, args to be passed to the called function
+/// @return {Promise} a promise encapuslaing the function
+function _promise(f,args) {


no single letter variables please, fn is a good alternative

shovon · 2016-11-27T16:53:32Z

bcrypt.js

+ //for node 0.12 stupidity
+ var _from = Array.from;
+ if(!_from)
+ _from = function(args) {


More nitpick: please wrap all subsequent statements inside curly brackets.

shovon · 2016-11-27T16:54:10Z

bcrypt.js

+ //cant do anything without promise
+
+ if(typeof Promise === 'undefined')
+ return;


Please wrap all subsequent statements inside curly brackets.

shovon · 2016-11-27T16:58:05Z

bcrypt.js

+ };
+
+ if(typeof args !== 'Array')
+ args = _from(args);


Same here. Wrap in curly braces please.

Is there a standard code style that you follow ?

Unfortunately, it looks like this repository does not have a specific code style outlined in any documents. You'll have to infer the style based on other code in this repository.

As far as my suggestions are concerned: terrible, terrible things have happened when people omit curly braces. One such example is the recent goto fail error that Apple had with their TLS suite. http://www.dwheeler.com/essays/apple-goto-fail.html#braces

I created a eslint code style to help me adhere to the style of the rest of the code.
Here is it: https://gist.github.com/Agathver/9762cc1c65878753e4e799348ee0411c

recrsn · 2016-11-27T17:03:28Z

Thanks for pointing out. Will be making changes to adhere to code style.

defunctzombie · 2016-11-27T17:54:52Z

Thank you. I also mirror @shovon in requesting that you add { } and avoid single line if statements. I prefer the consistency of just having { } for all if statements.

defunctzombie · 2016-11-27T18:21:00Z

bcrypt.js

+/// @param {function} function to be encapsulated
+/// @param {Array-like} args to be passed to the called function
+/// @return {Promise} a Promise encapuslaing the function
+var _promise = function (fn, args) {


doesn't need _ and can live in a separate file probably in a new lib folder

defunctzombie · 2016-11-27T18:21:26Z

bcrypt.js

 //just polyfill here (for 0.12)
- //this just wont work on node 0.12
+ //this just won't work on node 0.12
 //var _from = Array.from || [].slice.call; 

 //for node 0.12 stupidity
 var _from = Array.from;


_ are not needed, not sure what they are suppose to convey here.

defunctzombie · 2016-11-27T18:22:08Z

test/promise.js

+ },
+ test_hash_compare: function(assert) {
+ assert.expect(3);
+ bcrypt.genSalt(10, function(err, salt) {


where is the promise tested here?

defunctzombie · 2016-11-27T18:22:36Z

bcrypt.js

+var _promise = function (fn, args) {
+
+ //fn must be a function
+ var _self = this;


_ unnecessary

defunctzombie · 2016-11-27T18:22:55Z

bcrypt.js

+/// @return {Promise} a Promise encapuslaing the function
+var _promise = function (fn, args) {
+
+ //fn must be a function


this comment doesn't make sense to me here

defunctzombie · 2016-11-27T21:22:09Z

@ncb000gt thoughts? I am +1 on adding promises support.

@Agathver please update the README with documentation outlining the new promises support and how to use it.

charmander · 2016-11-28T00:46:02Z

lib/promises.js

+module.exports.promise = function (fn, context, args) {
+
+ //polyfill (for 0.12)
+ //avoid [].slice.call due to performance issues on v8


The comment says it avoids [].slice.call because of performance issues with arguments, then proceeds to use instead… [].slice.call. It doesn’t matter, though, because arguments is never passed to it, and that by itself would already be a deoptimization.

The code uses Array.from if available. But that is not available in node 0.12 so [].slice.call is used instead. args in most cases will be an arguments abject passed from another method.

The “performance problem” already exists when you pass arguments to exports.promise, and exists as much for Array.from as it exists for [].slice.call. Since the JavaScript part of the bcrypt bindings isn’t the slow one, though, it doesn’t really matter and you don’t need to comment on it here.

Removing the block altogether.

recrsn · 2016-11-28T07:05:06Z

@defunctzombie My first language is not English, so please review the updated README. I'll try my best to clarify if something is unclear.

defunctzombie · 2016-11-29T07:21:31Z

README.md

@@ -160,7 +205,7 @@ If you are using bcrypt on a simple script, using the sync mode is perfectly fin
 * `hash(data, salt, cb)`
 * `data` - [REQUIRED] - the data to be encrypted.
 * `salt` - [REQUIRED] - the salt to be used to hash the password. if specified as a number then a salt will be generated with the specified number of rounds and used (see example under **Usage**).
- * `cb` - [REQUIRED] - a callback to be fired once the data has been encrypted. uses eio making it asynchronous.
+ * `cb` - [OPTIONAL] - a callback to be fired once the data has been encrypted. uses eio making it asynchronous. If `cb` is not specified, a `Promise` is returned if Promise support is available.


Two spaces after If looks strange

defunctzombie · 2016-11-29T07:22:38Z

README.md

+const someOtherPlaintextPassword = 'not_bacon';
+```
+
+#### To hash a password:


I think you can avoid doing many examples and just say something along the lines of: any async method which takes a callback, can optionally return a promise if no callback is provided (or something of that nature). And maybe do a quick few line example. No need to go over all the api methods again.

defunctzombie · 2016-11-29T07:23:28Z

bcrypt.js

@@ -155,8 +164,16 @@ module.exports.compare = function(data, hash, cb) {
 });
 }

- if (!cb || typeof cb !== 'function') {
- return;
+ //cb exists but not a function


nitpick: space after the // I think that's how we do it in other places in the file.

defunctzombie · 2016-11-29T07:24:12Z

test/promise.js

+ assert.done();
+};
+
+//only run this tests if Promise is available


only run these tests if Promise is available

defunctzombie · 2016-11-29T07:24:41Z

test/promise.js

+var bcrypt = require('../bcrypt');
+
+var fail = function(assert, error) {
+ assert.ok(false, error);


need to require assert? Or is it already present globally?

assert is passed to test cases by nodeunit

defunctzombie · 2016-12-03T13:15:14Z

@Agathver great work and good follow through on all of the feed back!

I will give @ncb000gt through tomorrow to give any comments/review but I am happy with this and will merge and release on Sunday.

defunctzombie · 2016-12-03T13:16:19Z

@shovon @charmander any additional showstopper feedback?

shovon · 2016-12-03T20:54:50Z

@defunctzombie LGTM

recrsn added 2 commits November 27, 2016 20:57

add promise based interface

7da0f1e

add tests for promise based interface

45afd63

array ployfills for node 0.12.x and ignore promise for node <= 0.10.x

b2f24f6

defunctzombie reviewed Nov 27, 2016

View reviewed changes

fix some node 0.12 stupidity in polyfill

89e97f4

shovon suggested changes Nov 27, 2016

View reviewed changes

adhere to codestyle

60b4dfe

defunctzombie reviewed Nov 27, 2016

View reviewed changes

bcrypt.js

var _promise = function (fn, args) {

//fn must be a function

var _self = this;

Copy link

Collaborator

defunctzombie Nov 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_ unnecessary

defunctzombie reviewed Nov 27, 2016

View reviewed changes

move functions to separate file, fix tests

db0074f

charmander reviewed Nov 28, 2016

View reviewed changes

update README.md with new Promise api, fix some tests

c6e065f

defunctzombie reviewed Nov 29, 2016

View reviewed changes

fix docs

2410204

defunctzombie merged commit 2488473 into kelektiv:master Dec 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Promises #458

Add support for Promises #458

recrsn commented Nov 27, 2016 •

edited

Loading

mention-bot commented Nov 27, 2016

recrsn commented Nov 27, 2016

defunctzombie Nov 27, 2016

defunctzombie Nov 27, 2016 •

edited

Loading

shovon Nov 27, 2016

shovon Nov 27, 2016

shovon Nov 27, 2016

recrsn Nov 27, 2016

shovon Nov 27, 2016 •

edited

Loading

recrsn Dec 4, 2016

recrsn commented Nov 27, 2016

defunctzombie commented Nov 27, 2016

defunctzombie Nov 27, 2016

defunctzombie Nov 27, 2016

defunctzombie Nov 27, 2016

defunctzombie Nov 27, 2016

defunctzombie Nov 27, 2016

defunctzombie commented Nov 27, 2016

charmander Nov 28, 2016

recrsn Nov 28, 2016

charmander Nov 28, 2016

recrsn Nov 28, 2016

recrsn commented Nov 28, 2016

defunctzombie Nov 29, 2016

defunctzombie Nov 29, 2016

defunctzombie Nov 29, 2016

defunctzombie Nov 29, 2016

defunctzombie Nov 29, 2016

recrsn Nov 29, 2016

defunctzombie commented Dec 3, 2016

defunctzombie commented Dec 3, 2016

shovon commented Dec 3, 2016

Add support for Promises #458

Add support for Promises #458

Conversation

recrsn commented Nov 27, 2016 • edited Loading

mention-bot commented Nov 27, 2016

recrsn commented Nov 27, 2016

Choose a reason for hiding this comment

defunctzombie Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shovon Nov 27, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recrsn commented Nov 27, 2016

defunctzombie commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defunctzombie commented Nov 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

recrsn commented Nov 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defunctzombie commented Dec 3, 2016

defunctzombie commented Dec 3, 2016

shovon commented Dec 3, 2016

recrsn commented Nov 27, 2016 •

edited

Loading

defunctzombie Nov 27, 2016 •

edited

Loading

shovon Nov 27, 2016 •

edited

Loading