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

Refetch even when the query has not changed #13759

Merged
merged 5 commits into from
Sep 7, 2017

Conversation

lukasolson
Copy link
Member

Fixes #13743.

This PR makes it so any time a query is submitted (in Discover/Visualize/Dashboard), a fetch happens, even if the query itself doesn't change.

Copy link
Contributor

@Bargs Bargs left a comment

Choose a reason for hiding this comment

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

LGTM. Just left one minor suggestion about function names.

@@ -465,8 +461,8 @@ function discoverController(
if ($state.query.language && $state.query.language !== query.language) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I called these methods updateQuery since they only update state. Now that they also fetch results, maybe we should call them something like fetchWithQuery?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍 9c27c33

Copy link
Contributor

Choose a reason for hiding this comment

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

That name sounds good to me! Did you forget to update all the usages though?

Copy link
Member Author

Choose a reason for hiding this comment

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

How embarrassing. ef09fa0

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

It seems to fix the issue, but leads to updateQueryAndFetch being called twice when the query has changed: once for the "submit" and once triggered by the $watch('state.query', ...). Is there maybe some way to get rid of that $watch?

@Bargs
Copy link
Contributor

Bargs commented Aug 31, 2017

For background, the watch is there in order to ensure old queries get migrated. Queries on saved searches get migrated at load time, but a query might also be stored in appstate in the url of a bookmark or link. It was the only thing I could think of to ensure the app is always working with an updated query object.

@lukasolson
Copy link
Member Author

@weltenwort Invoking the function multiple times doesn't result in multiple HTTP requests, because of how the courier works. That being said, I agree there is a bit of code smell there, but the only thing I can think of to prevent this would be to manually check if the query object has changed and return early if it has (since it will be called inside the watch). Thoughts?

Copy link
Member

@weltenwort weltenwort left a comment

Choose a reason for hiding this comment

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

After thinking some more about it I can't think of an elegant fix either. Relying on the deduplication feature of courier is probably the way to go here.

@lukasolson lukasolson merged commit d331a7e into elastic:master Sep 7, 2017
lukasolson added a commit to lukasolson/kibana that referenced this pull request Sep 7, 2017
* Refetch even when the query has not changed

* Change function name to better represent what it does

* Rename all occurrences
lukasolson added a commit to lukasolson/kibana that referenced this pull request Sep 7, 2017
* Refetch even when the query has not changed

* Change function name to better represent what it does

* Rename all occurrences
@lukasolson
Copy link
Member Author

6.x (6.1.0): eb86cff
6.0 (6.0.0): 0151396

@lukasolson lukasolson deleted the updateQuery branch March 27, 2018 21:09
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