Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix query input lag #21753

Merged
merged 2 commits into from
Aug 10, 2018
Merged

Fix query input lag #21753

merged 2 commits into from
Aug 10, 2018

Conversation

Bargs
Copy link
Contributor

@Bargs Bargs commented Aug 7, 2018

Fixes #20486

The debounce in ng-model-options was intended to avoid updating the
typeahead suggestions too often. It had the side effect of delaying
query input updates to the model on the angular scope. As a result, if
you typed quickly and hit enter within 100ms, you would submit an out of
date query. This moves the debounce from ng-model-options to the
updateSuggestions method itself.

The debounce in ng-model-options was intended to avoid updating the
typeahead suggestions too often. It had the side effect of delaying
query input updates to the model on the angular scope. As a result, if
you typed quickly and hit enter within 100ms, you would submit an out of
date query. This moves the debounce from ng-model-options to the
updateSuggestions method itself.
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@timroes
Copy link
Contributor

timroes commented Aug 8, 2018

I can confirm this fixes the bug. There is a minor "issue" with the code. If you navigate really fast away from the page while the debounce is still running we'll still trigger a debounce even while we are already on a separate page. Since the debounced function then access the already destroyed scope (calling $apply) on it, I am not sure what the outcome is. To make this 100% safe, we would need to cancel the debounce if the scope gets destroyed and since there is another async operation in the debounce, also check after that again if we are still "alive":

this.updateSuggestions = debounce(async () => {
  const suggestions = await this.getSuggestions();
  if (!this._destroyed) {
    $scope.$apply(() => this.suggestions = suggestions);
  }
});

$scope.$on('$destroy', () => {
  this.updateSuggestions.cancel();
  this._destroyed = true;
});

I know that code usually is super ugly to read, but also these issues are super hard to detect later on in case they appear, why I think even with a small debounce of 100ms we should rather go for safe cod. We actually hit that issue a couple of times with our render debounce, that is also at 100ms.

@Bargs
Copy link
Contributor Author

Bargs commented Aug 10, 2018

Good point @timroes, I've made the update you suggested.

@Bargs Bargs removed the request for review from lukasolson August 10, 2018 13:17
Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm, that the reported issue is fixed by this. Code looks also good to me.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@Bargs Bargs merged commit be23ce4 into elastic:master Aug 10, 2018
Bargs added a commit to Bargs/kibana that referenced this pull request Aug 10, 2018
The debounce in ng-model-options was intended to avoid updating the
typeahead suggestions too often. It had the side effect of delaying
query input updates to the model on the angular scope. As a result, if
you typed quickly and hit enter within 100ms, you would submit an out of
date query. This moves the debounce from ng-model-options to the
updateSuggestions method itself.
Bargs added a commit that referenced this pull request Aug 10, 2018
The debounce in ng-model-options was intended to avoid updating the
typeahead suggestions too often. It had the side effect of delaying
query input updates to the model on the angular scope. As a result, if
you typed quickly and hit enter within 100ms, you would submit an out of
date query. This moves the debounce from ng-model-options to the
updateSuggestions method itself.
@Bargs Bargs mentioned this pull request Aug 13, 2018
@spalger spalger removed the v6.6.0 label Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants