fix(autocomplete): improve promise logic

[winniehell]

The finally-function supported by q is not part of the A+ spec. Therefore, other promise implementations do not have it (e.g. the promise polyfill in babel). To ensure that md-autocomplete resets its loading state properly, I used then if finally is not available.

This pull request was joined with #6860.

[googlebot]

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

---

• If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
• If you signed the CLA as a corporation, please let us know the company's name.

[winniehell]

@googlebot I signed it!

[googlebot]

CLAs look good, thanks!

[winniehell]

Failing tests seem to be unrelated as they appear e.g. in #6515 (build 99782625) and #6518 (build 99851677) as well. Not sure what I can do about it... 👽❓

[devversion]

The tests of the whole project are currently failing. That's not an issue from you ;)

And now to the PR, I thought we using here the Promise Service from Angular. See here. This a implementation inspired by https://github.com/kriskowal/q

Am I wrong?

[winniehell]

@devversion you are right that it is usual to use $q from angular in that context—but it is not necessary to do so. In my opinion, md-autocomplete should work with other promise implementations as well (and besides this little issue, it seems to me that it actually does 👍).

There are two reasons for me not to use $q from angular:

1. it is common to use the Deferred API which is not part of the A+ spec and deprecated for other implementations (e.g. Gecko)
2. when combining angular with another framework (e.g. meteor), always wrapping existing promises with $q is cumbersome 🌵

[devversion]

Can be true, I don't know about A+ etc. But in this context of code we using DI to get the $q service of Angular, so there won't be any other Deferred API, that's why I think this change is not needed.

[winniehell]

Even though $q is injected, items is evaluated from the expression which is passed to the directive. Therefore it can basically be any expression. However, md-autocomplete only works with array expressions and thenables. Currently, it is restricted to thenables with a finally-function. In my opinion, this restriction is not necessary.

[winniehell]

For clarification: the effect on md-autocomplete for thenables which do not have a finally-function is that setLoading(false) is never called when no matching item can be found. As a result instead of showing md-not-found the loading indicator remains visible.

[winniehell]

Tests are now all green. ?

[ThomasBurleson]

Why not use the $q.when( ) to wrap angular & foreign promises:

function fetchResults (searchText) {
  var items = $scope.$parent.$eval(itemExpr),
      term  = searchText.toLowerCase(),
      isList = angular.isArray(items);

  if ( isList ) handleResults(items);
  else          handleAsyncResults(items);

  function handleAsyncResults(items) {
    if ( !items ) return;

    items = $q.when(items);
    setLoading(true);

    $mdUtil.nextTick(function () {      
        items.finally(function(){ 
          setLoading(false); 
        });
    },true, $scope);
  }
}

[ThomasBurleson]

PR #6860 also suggest not listening for .success( ).

[robertmesserle]

@ThomasBurleson Probably a good point, but this is a complaint on the original code, not this PR.

[ThomasBurleson]

@winniehell - please incorporate my suggestions ^^ and test. This will make the merged code better moving forward.

[ThomasBurleson]

@robertmesserle - can you review?

[robertmesserle]

@ThomasBurleson LGTM

[winniehell]

@ThomasBurleson: I'll try to rework this including your comments and a fix for #6860.

[winniehell]

@ThomasBurleson: Alright, I was still trying to figure out, how to write a test for it. Thank you for fixing this! 👍 😃