Fix for compare method not rejecting promise on error #469
Conversation
|
Needs tests (didn't notice if those were added due to the diff noise) |
|
I removed the unnecessary changes. |
| return process.nextTick(function() { | ||
| cb(new Error('data and hash arguments required')); | ||
| data(new Error('data and hash arguments required')); |
There was a problem hiding this comment.
this doesn't really match the if condition. The user might provide a function for the data argument and then see this error when in fact they did provide an argument.
This also doesn't handle the case where the user passes null for the arguments and does not specify a callback. They will not get a rejection.
The real mistake of the API is to not throw then it is called with the wrong arguments which is what should have happened but given that is not the case. Fixing this to match previous behavior means identifying the error condition and then rejecting with promise or callback as needed.
There was a problem hiding this comment.
I tried to make it identical to hash, where it is assumed that if the user passes a function in place of parameters, the data and salt arguments are missing and the passed function is the callback which should be called with the error.
May be the error message could be rewritten as 'data and hash must be strings' ?
If the user passes null for either the data or hash, and does not specify a callback, the Promise is rejected.
The previous (v0.8.7) behavior was to fail with TypeError: cb is not a function, But hash used to call the passed method and silently fail if no callback was specified.
There was a problem hiding this comment.
There is no check now in this function for (data == null || hash == null) when no callback is provided (also in the hash function). Is this not going to be an issue?
There was a problem hiding this comment.
When no callback is provided this function returns a promise (which is made simply from providing a callback and calling again) and then the null checks are present and the promise is rejected.
There was a problem hiding this comment.
You're right, I missed that it's simply calling itself again. Thanks!
| return process.nextTick(function() { | ||
| cb(new Error('data and hash arguments required')); | ||
| data(new Error('data and hash arguments required')); |
This pull request aims to fix #468 - Compare method not rejecting promise on error