Older look-ups draw over newer lookups

[vwbusguy]

If an earlier query returns before the a latter query has already been resolved, it will overwrite the new query with the older query responses in the autocomplete population, and will even do this if an item was already selected.

For example: https://github.com/matiasdelellis/facerecognition/issues/766

Older lookups should be abandoned if a newer lookup has resolved and if an item has already been selected, it should also not re-populate the suggestions, which can cause an unintended selection.

[realsuayip]

The autocomplete constructor takes lookup argument, which is described as:

Takes name, returns a Promise that resolves into a list of {name, value}s.

Since that promise resolving is async in nature, the library cannot know which promise resolved when (or which ones are valid).

In this case, I suggest you use some kind of cancellation logic and reject the previous request's promise, so that only the latest request resolves. Rejection of lookup promise seems to be no-op in the library.

[vwbusguy]

Unfortunately, I do not maintain the Nextcloud facerecognition code that uses this library.

I solved a related problem in an unrelated codebase recently by checking against a counter that incremented with each request, and while Javascript Promise does have a way to reject other calls once a newer one comes in, I'm not immediately sure how to do this the way this is currently written where each input creates a new Promise, besides delaying the call after initial input and aggregating the inputs and then sending in the last input or sending all to the promise with the .any().

Another possibility (it's a bit old school) that would work with the code as it is now is to set a global (or sufficiently highly scoped) var on Lookup (before calling Promise) and on resolve, check that var to see if it's still set and if so, update the field and reset it, and if not, abandon the result.

[vwbusguy]

I tried it out and while it's not a perfect solution, it's definitely an improvement. I think it can be further improved with a proper counter.

var autocomplete_req = 0;

...
new AutoComplete({
                                lookup (query) {
                                        autocomplete_req = 1;
                                        return new Promise(resolve => {
                                                $.get(some_lookup(query)).done(function (names) {
                                                        if (autocomplete_req == 1) {
                                                                autocomplete_req = 0;
                                                                resolve(names);
                                                        }
                                                });
                                        });
                                },

[vwbusguy]

Got it to work with a counter and now things are working as expected on the browser side of things!

Here's the gist:

var autocomplete_cnt = 0;
var autocomplete_lst = 0;
new AutoComplete({
  lookup (query) {
    let autocomplete_cnt_itr = ++autocomplete_cnt;
    return new Promise(resolve => {
      some_lookup(query).done(function (result) {
        if (autocomplete_cnt_itr > autocomplete_lst) {
            autocomplete_lst = autocomplete_cnt_itr;
            resolve(result);
        }
      });

This is an example of it working with some variables outside of the object, but it would be cleaner for this to happen within the library since this would need to be repeated wherever this is instantiated with unique counter variables for each declaration otherwise.

[realsuayip]

If I understood correctly, the need for keeping global state arises from the fact that the autocomplete object gets destroyed with the input. The library does tie cache with input since managing outer global state via local storage or something similar might get a bit tedious (for example, imagine multiple autocomplete inputs in the same page -- you would also need globally unique identifiers for each input across your website).

I would suggest, instead of destroying the input completely, maybe hide it from the DOM instead? That's what I did with my modals that contained autocomplete inputs.

For the lookup race, I still suggest using some kind of promise rejection strategy. The library innately cannot know which promise to resolve first. What if some other users do want to resolve the lookup even if the response is delayed?

[vwbusguy]

If I understood correctly, the need for keeping global state arises from the fact that the autocomplete object gets destroyed with the input.

The scope doesn't actually need to be global - just higher than the scope of the lookup function. It would be more appropriate as an object variable set on init for AutoComplete, IMO. The higher scoped var is just a workaround for that not existing in this library as it currently is.

I would suggest, instead of destroying the input completely, maybe hide it from the DOM instead? That's what I did with my modals that contained autocomplete inputs.

Unfortunately, I don't own that downstream code, but I did submit a PR to fix it based on the current implementation. So, for now, this is working on my server environment, at least, but I would love to see a fix for other users of these things.

For the lookup race, I still suggest using some kind of promise rejection strategy. The library innately cannot know which promise to resolve first. What if some other users do want to resolve the lookup even if the response is delayed?

The promise rejection strategy would make sense if the calls were tied to the same promise but the current code for this library creates a new promise for every lookup. Are you suggesting that a user might want something other than their most recent input as reflected in the input? I definitely did not expect this behavior as a user, nor for the later result to draw over and replace my current selected result.

---

Just because tone often gets lost online, I also want to say that I appreciate your work on this library and the downstream's integration of it. This is most definitely a nice feature to have that has saved me a lot of error in trying to organize my family pictures, so thanks :-) .