Why is nextTick() used on all error callbacks in bcrypt.js?

[tonylukasavage]

system

• bcrypt (master - 054cf76)
• node 8.4.0

problem

In bcrypt.js there is a pattern where all error callbacks are wrapped in a nextTick() call, like this:

return process.nextTick(function() {
    return cb(new Error('rounds must be a number'));
});

In the case of Promises, this cb is the callback that bcrypt creates internally to either resolve or reject the call. On a rejection I get a stack trace that looks like this:

Error: rounds must be a number
    at /node_modules/bcrypt/bcrypt.js:54:15
    at _combinedTickCallback (internal/process/next_tick.js:131:7)
    at process._tickDomainCallback (internal/process/next_tick.js:218:9)

As you can see, the call stack is essentially trashed by the nextTick() call. I get no reference to the code in my project that actually invoked bcrypt making this extremely difficult to troubleshoot. If I remove the nextTick() call and run it again, I get a stack trace like this:

Error: rounds must be a number
    at Object.genSalt (/node_modules/bcrypt/bcrypt.js:54:15)
    at /node_modules/bcrypt/lib/promises.js:32:12
    at Promise (<anonymous>)
    at Object.module.exports.promise (/node_modules/bcrypt/lib/promises.js:23:12)
    at Object.genSalt (/node_modules/bcrypt/bcrypt.js:43:25)
    at Object.passwordHash (/lib/security.js:16:26)
    at Context.it.only (/test/lib/security_test.js:20:19)
    at callFn (/node_modules/mocha/lib/runnable.js:326:21)
    at Test.Runnable.run (/node_modules/mocha/lib/runnable.js:319:7)
    at Runner.runTest (/node_modules/mocha/lib/runner.js:422:10)
    at /node_modules/mocha/lib/runner.js:528:12
    at next (/node_modules/mocha/lib/runner.js:342:14)
    at /node_modules/mocha/lib/runner.js:352:7
    at next (/node_modules/mocha/lib/runner.js:284:14)
    at Immediate._onImmediate (/node_modules/mocha/lib/runner.js:320:5)
    at runCallback (timers.js:781:20)
    at tryOnImmediate (timers.js:743:5)
    at processImmediate [as _immediateCallback] (timers.js:714:5)

So as you can see, I need a stack trace that looks like the latter. This is true of ALL bcrypt API calls, they all have this pattern when handling errors.

expected

I want a stack trace that looks like the complete one I posted above when encountering an error using Promises with the bcrypt API. I believe this is possible by simply removing the nextTick() wrapper around the error callback invocations.

I'm sure I'm ignorant of something being new to this code base, but why are the nextTick() calls necessary? If it is OK to remove them, I'd be happy to submit a PR.

[tonylukasavage]

Apparently I'm late to provide a counter point to this issue and merge, but here I am now, better late than never: #264

[tonylukasavage]

In digging around a little bit, it could be really simple to keep the nextTick() as while still getting the proper stack trace. You could just move the new Error() call outside of the nextTick(). So instead of:

return process.nextTick(function() {
    cb(new Error('rounds must be a number'));
});

you could do this, which would preserve the stack trace and still makes the callback async as mentioned in #264:

let error = new Error('rounds must be a number');
return process.nextTick(function() {
    cb(error);
});

Would a PR that did this be acceptable?