algolia/algoliasearch-client-js — pull request #11 by ExplodingCabbage (2014-05-18 22:09:41+00:00)
Title: Make AlgoliaSearch.prototype.search callback asynchronous always

Previously, it was usually an async callback but became synchronous
when the result was cached in this.cache. This violates best practice
that callbacks should either be always-sync or always-async.

For an oft-cited source of the idea that callbacks should either be consistently sync or consistently async, see http://blog.izs.me/post/59142742143/designing-apis-for-asynchrony. For a concrete example of how this inconsistent behaviour can be harmful in the case of this library, consider the code below (which is roughly equivalent to a real bug I had to figure out recently in some code that was Algolia):

```js
var searchHelper = new AlgoliaSearchHelper( ... );

// ...

$searchBox.change(function () {
searchHelper.search(
$searchBox.val(),
function successHandler () {
hideLoadingSpinner();
// ...
// (some other stuff)
}
);
showLoadingSpinner();
})
```

The subtle bug my colleague created here (that if you search for something you've searched for already, the callback fires *before* the change handler returns and so the loading spinner never disappears) seems like a trap that others are likely to fall into as well, and it would not be possible with the change I've suggested in this commit, since the callback now *always* happens asynchronously.