client.variation should not release zalgo

[jvivs]

client.variation has an error-first callback interface, it unfortunately does not appear to follow the pattern completely.

Instead of always calling callbacks asynchronously, callbacks in the variation method could be called either sync or async.

This is a serious issue that introduces inconsistencies in code execution and impedes the ability for consumers to be able to reason about your. Without following this pattern to the letter, you release zalgo.

[apucacao]

Hi @jvivs ,

Thanks for bringing this up. This is important to get right. We're going to change the code to avoid this inconsistency.

[apucacao]

Hi @jvivs ,

Our latest release — which adds Promise support — fixes this problem altogether.

Thanks for bringing it up!

Best,
Alexis

[johnvh]

The convention for callbacks is to wrap them in a process.nextTick() to force async. This should be trivial to do, no?

It seems reasonable to me that, if you're going to support callbacks, you should fix this. Many still prefer to use callbacks over promises.

[apucacao]

Our Promise-based solution should be compatible with both callbacks and promises.

Is that not true? Did you run into an issue?

[johnvh]

I thought the original issue would still be present, but I did not look close enough at the implementation - my bad. You go through a promise, which does force async.

However, this does mean that this module is no longer compatible with node 0.8.x - unless global.Promise is polyfilled. Minor issue, but you may want to update engines.node in package.json to reflect that.