No errors thrown by asynchronous functions #168

Merged
merged 1 commit into from Jun 13, 2013

Projects

None yet

3 participants

@shovon
Contributor
shovon commented May 26, 2013

This tackles issue #167.

Collaborator

Not sure how I feel about this actually. On one hand it makes sense and on
the other the calls being made are invalid. I think we are also being too
defensive in our checks.
On May 26, 2013 3:15 AM, "Salehen Shovon Rahman" notifications@github.com
wrote:

This tackles issue #167#167

.

You can merge this Pull Request by running

git pull https://github.com/shovon/node.bcrypt.js async-except

Or view, comment on, or merge it at:

#168
Commit Summary

  • No errors thrown by asynchronous functions.

File Changes

  • M bcrypt.jshttps://github.com/ncb000gt/node.bcrypt.js/pull/168/files#diff-0(28)
  • M test/async.jshttps://github.com/ncb000gt/node.bcrypt.js/pull/168/files#diff-1(34)

Patch Links:

Collaborator

I agree with @shovon here. We have to make sure that we do things the "node way". While callbacks aren't exactly a node thing, making the first param to the callbacks the error param is. So, this, as @shtylman says, "makes sense".

@shtylman would you mind elaborating on your reasons?

Collaborator

Basically, I think our checks for callback functions are a bit pedantic as no libs typically do this. I will also note that if we used "asserts" instead, then those would throw. Honestly, while it is unfortunate, you should expect that sometimes things will throw :/

Contributor
shovon commented May 28, 2013

I have to admit, the checks for callbacks are much more defensive than it should be.

I might be wrong, but a lot of libs actually don't throw errors, if we forget to pass in a callback. Take Node.js' own fs.readFile function. Just call it by passing in the two first parameters, and omit the callback. And no errors will be thrown.

var fs = require('fs');

fs.readFile('something', 'utf8');

I'm not a 100% sure how do those libs achieve it. The only way that I can think of is doing a type check:

var myAsyncFunc = function (someParam, callback) {
  if (typeof callback !== 'function') {
    return;
  }

  // do some computation here...
};

But that's being defensive--albeit, not as defensive as I was in this pull request. I don't know of any other way to prevent throwing errors in the absence of a callback.

Edit: I refined the last paragraph.

Collaborator

Most typically do cb = cb || function () {};

But I think that is defensive as well.

On Mon, May 27, 2013 at 8:50 PM, Salehen Shovon Rahman <
notifications@github.com> wrote:

I have to admit, the checks for callbacks are much more defensive than it
should be.

I might be wrong, but a lot of libs actually don't throw errors, if we
forget to pass in a callback. Take Node.js' own fs.readFile function.
Just call it by passing in the two first parameters, and omit the callback.
And no errors will be thrown.

var fs = require('fs');
fs.readFile('something', 'utf8');

I'm not a 100% sure how do those libs achieve it. The only way that I can
think of is doing a type check:

var myAsyncFunc = function (someParam, callback) {
if (typeof callback !== 'function') {
return;
}

// do some computation here...};

But that's being defensive, and I don't know of any other way to prevent
throwing errors in the absence of a callback.


Reply to this email directly or view it on GitHubhttps://github.com/ncb000gt/node.bcrypt.js/pull/168#issuecomment-18524318
.

Collaborator

So, thoughts about merging this in?

This originated from how things were done in the C++ code. When we moved the checks out the JS land it was purely a backwards compatibility thing afaik.

I could go either way on this.

Collaborator

After thinking about it some more, I think this is a good way to go. Error handling in node/js is always going to be terrible and at least this way we can isolate it how users expect. Will review today and merge barring any major issues.

Collaborator

Sounds good. Thanks.

  • Nick Campbell

http://digitaltumbleweed.com

On Wed, Jun 12, 2013 at 12:57 PM, Roman Shtylman
notifications@github.comwrote:

After thinking about it some more, I think this is a good way to go. Error
handling in node/js is always going to be terrible and at least this way we
can isolate it how users expect. Will review today and merge barring any
major issues.


Reply to this email directly or view it on GitHubhttps://github.com/ncb000gt/node.bcrypt.js/pull/168#issuecomment-19339457
.

@defunctzombie defunctzombie merged commit a7c7e0a into kelektiv:master Jun 13, 2013

1 check passed

default The Travis CI build passed
Details
Collaborator

published as 0.7.6

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment