Skip to content

Conversation

@brendanashworth
Copy link
Contributor

In many of the functions in bcrypt.js, there was a lackluster
adherence to the concept of sync OR async APIs: in many of the
supposedly asynchronous functions, the callbacks would be called
synchronously, when they should always be called asynchronously.

See:

With this commit, this has been fixed by appropriately using
process.nextTick to asynchronously call the callback instead.

In many of the functions in `bcrypt.js`, there was a lackluster
adherence to the concept of sync OR async APIs: in many of the
supposedly asynchronous functions, the callbacks would be called
synchronously, when they should always be called asynchronously.

See:
- http://blog.ometer.com/2011/07/24/callbacks-synchronous-and-asynchronous/
- http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony
- https://www.joyent.com/developers/node/design/errors

With this commit, this has been fixed by appropriately using
`process.nextTick` to asynchronously call the callback instead.
@defunctzombie
Copy link
Collaborator

meh, I don't think this matters that much. I am aware of those blog posts but don't really buy the argument that you have to do it that way. In my experience it has not led to confusion.

@brendanashworth
Copy link
Contributor Author

Well, the API gives both synchronous and asynchronous versions of itself. It doesn't make much sense to have the sync API be completely sync and then async API to be part async and part sync... it should be one or the other.

Also, some applications rely on completing code after the asynchronous function is launched but before a callback is invoked, and invoking the callback synchronously would break this promise between the API, which claims itself to be asynchronous, and the actual implementation, which is synchronous.

Regardless, it is specified by Joyent to be the preferred standard for APIs. If you think that the errors should still be handled synchronously rather than asynchronously, we can always instead resort to throw new TypeError('...'); in the code, which is what many other libraries do (see: fs.js).

@defunctzombie
Copy link
Collaborator

Callback API simply means that the callback can happen at a later time and that you do not know exactly when. I personally do not feel that strongly about it nor do I think it has been an issue for uses of this library. If @ncb000gt feels differently we can merge this. I think it is not useful.

@ncb000gt
Copy link
Member

Although I don't, nor do most people using this module afaict, find this to be an issue, I don't mind merging this in. It does make the async api purely async and that's probably a good thing overall.

ncb000gt added a commit that referenced this pull request Dec 24, 2014
@ncb000gt ncb000gt merged commit 05e2513 into kelektiv:master Dec 24, 2014
@brendanashworth
Copy link
Contributor Author

Thanks!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants