fix(autocomplete): stop loading if last promise got resolved #6927

devversion · 2016-01-30T10:07:52Z

As said in #6907:

Just investigated a bit, the problem is, the autocomplete will set the loading state to false if a previous promise is resolved. So when you search for A-R-I-Z-O-N-A (normally typed - not pasted) there will be a promise for each char. So the state of setLoading will change to false. This can be solved by adding a simple fetching queue. But only check the queue on the promise finally to store the retrieved results in cache.

-- Little notice.

I would like to submit a test for that, but that is complicated for two promises, where the first promise should be flushed (only the first) or resolved. But that will be easy to solve, just resolving the promises manually. But then there will be another problem, because there is another timeout running from the autoComplete controller, which validates the promises in the next tick. So flushing and manually resolving won't work, to test this new feature / issue.

Fixes #6907

ThomasBurleson · 2016-01-30T17:13:39Z

@robertmesserle - please review/test/authorize.

kseamon · 2016-02-02T16:36:34Z

src/components/autocomplete/js/autocompleteController.js

- promiseFetch = false;
+ if (--fetchesInProgress === 0) {
+ setLoading(false);
+ promiseFetch = false;


I think it would be cleaner to have only one variable that tracks the fetching state. I'd get rid of promiseFetch and just use fetchesInProgress. If need be, add a getter method that just returns fetchesInProgress === 0

Yes definitely 👍 Just had no chance to look over, because of the rebased 52a519e commit.

@kseamon Just want to let you know 😄 - Done!

kseamon · 2016-02-02T17:09:57Z

@ThomasBurleson LGTM

devversion · 2016-02-06T17:17:51Z

Rebased for 4d59a61

> Just investigated a bit, the problem is, the autocomplete will set the loading state to false if a previous promise is resolved. So when you search for A-R-I-Z-O-N-A (normally typed - not pasted) there will be a promise for each char. So the state of setLoading will change to false. This can be solved by adding a simple fetching queue. But only check the queue on the promise finally to store the retrieved results in cache. Fixes angular#6907

> Just investigated a bit, the problem is, the autocomplete will set the loading state to false if a previous promise is resolved. So when you search for A-R-I-Z-O-N-A (normally typed - not pasted) there will be a promise for each char. So the state of setLoading will change to false. This can be solved by adding a simple fetching queue. But only check the queue on the promise finally to store the retrieved results in cache. Fixes #6907 Closes #6927 # Conflicts: # src/components/autocomplete/js/autocompleteController.js

> Just investigated a bit, the problem is, the autocomplete will set the loading state to false if a previous promise is resolved. So when you search for A-R-I-Z-O-N-A (normally typed - not pasted) there will be a promise for each char. So the state of setLoading will change to false. This can be solved by adding a simple fetching queue. But only check the queue on the promise finally to store the retrieved results in cache. Fixes angular#6907 Closes angular#6927

ThomasBurleson added the needs: review This PR is waiting on review from the team label Jan 30, 2016

ThomasBurleson added this to the 1.0.5 milestone Jan 30, 2016

ThomasBurleson assigned robwormald and robertmesserle and unassigned robwormald Jan 30, 2016

ThomasBurleson added the in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs label Jan 30, 2016

devversion force-pushed the fix/autocomplete-lastpromise-resolve branch from 8a0971f to 5332162 Compare February 2, 2016 12:32

kseamon reviewed Feb 2, 2016
View reviewed changes

devversion force-pushed the fix/autocomplete-lastpromise-resolve branch from 5332162 to 14026f1 Compare February 2, 2016 17:06

ThomasBurleson modified the milestones: 1.0.5, 1.0.6 Feb 4, 2016

ThomasBurleson assigned ThomasBurleson and unassigned robertmesserle Feb 6, 2016

ThomasBurleson added pr: merge ready This PR is ready for a caretaker to review and removed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: review This PR is waiting on review from the team labels Feb 6, 2016

devversion force-pushed the fix/autocomplete-lastpromise-resolve branch from 14026f1 to f168ccb Compare February 6, 2016 17:17

devversion added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Mar 21, 2016

devversion force-pushed the fix/autocomplete-lastpromise-resolve branch from f168ccb to 6feafc1 Compare March 21, 2016 20:48

devversion removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Mar 21, 2016

ThomasBurleson closed this in e372cf9 Mar 30, 2016

devversion deleted the fix/autocomplete-lastpromise-resolve branch April 21, 2016 12:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(autocomplete): stop loading if last promise got resolved #6927

fix(autocomplete): stop loading if last promise got resolved #6927

devversion commented Jan 30, 2016

ThomasBurleson commented Jan 30, 2016

kseamon Feb 2, 2016

devversion Feb 2, 2016

devversion Feb 2, 2016

kseamon commented Feb 2, 2016

devversion commented Feb 6, 2016

fix(autocomplete): stop loading if last promise got resolved #6927

fix(autocomplete): stop loading if last promise got resolved #6927

Conversation

devversion commented Jan 30, 2016

ThomasBurleson commented Jan 30, 2016

kseamon Feb 2, 2016

Choose a reason for hiding this comment

devversion Feb 2, 2016

Choose a reason for hiding this comment

devversion Feb 2, 2016

Choose a reason for hiding this comment

kseamon commented Feb 2, 2016

devversion commented Feb 6, 2016