Accommodate @client @export variable changes in ObservableQuery
#4604
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
requested changes
Mar 20, 2019
When using `client.watchQuery` to setup an Observable against a query that's using `@client @export` directives, cache changes could happen that lead to the value of the `@client @export` variable changing. When this happens, if the watched query is using a mixture of local and remote data, the watched query itself might become invalid, since the `@client @export` variable used to fetch the data has now changed. To address this, this commit updates the `ObservableQuery` observers such that they first check to see if `@client @export` directives are being used, and update any associated variables. It will then perform a `refetch` using the new variables if needed, otherwise it will let the observers continue with their normal behaviour.
7d0cfd0 to
a8f944d
Compare
benjamn
approved these changes
Apr 5, 2019
benjamn
reviewed
Apr 5, 2019
| // changed, and the query is calling against both local and remote | ||
| // data, a refetch is needed to pull in new data, using the | ||
| // updated `@export` variables. | ||
| if (queryManager.transform(query).hasClientExports) { |
There was a problem hiding this comment.
By using cached data from queryManager.transform(query) we can avoid repeatedly calling hasClientExports or queryManager.getLocalState().serverQuery (below).
benjamn
reviewed
Apr 5, 2019
| @@ -713,7 +713,7 @@ export class QueryManager<TStore> { | |||
| }> | |||
| >(); | |||
|
|
|||
| private transform(document: DocumentNode) { | |||
| public transform(document: DocumentNode) { | |||
There was a problem hiding this comment.
I don't love making this public, but at least the result type is read-only.
benjamn
added a commit
that referenced
this pull request
Apr 5, 2019
hwillson
added a commit
that referenced
this pull request
Feb 14, 2020
PR #4604 fixed an issue where changes made to `@client @export` based variables didn't always result in re-running the query that was using the variable. Unfortunately, these changes are a bit too aggressive. They're triggering a `refetch` when an `@client @export` based variable changes (and a few additional conditions are met), and while using the `ObservableQuery.refetch()` method does fix the original issue, it leads to a situation where the query being re-run is always fired over the network, even if the updated query could have been resolved from the cache. `ObservableQuery.refetch()` forces queries to use a network based fetch policy, which means if the query was originally fired with a `cache-first` policy (for example), and has matching data in the cache after taking into consideration the new variable, that data won't be used and instead an extra unnecessary network request will fire. This commit addresses the issue by leveraging `ObservableQuery.setVariables()` instead of automatically refetching. `setVariables` will attempt to resolve the updated query from the cache first, then only fire it over the network if required.
hwillson
added a commit
that referenced
this pull request
Feb 14, 2020
PR #4604 fixed an issue where changes made to `@client @export` based variables didn't always result in re-running the query that was using the variable. Unfortunately, these changes are a bit too aggressive. They're triggering a `refetch` when an `@client @export` based variable changes (and a few additional conditions are met), and while using the `ObservableQuery.refetch()` method does fix the original issue, it leads to a situation where the query being re-run is always fired over the network, even if the updated query could have been resolved from the cache. `ObservableQuery.refetch()` forces queries to use a network based fetch policy, which means if the query was originally fired with a `cache-first` policy (for example), and has matching data in the cache after taking into consideration the new variable, that data won't be used and instead an extra unnecessary network request will fire. This commit addresses the issue by leveraging `ObservableQuery.setVariables()` instead of automatically refetching. `setVariables` will attempt to resolve the updated query from the cache first, then only fire it over the network if required.
hwillson
added a commit
that referenced
this pull request
Feb 14, 2020
PR #4604 fixed an issue where changes made to `@client @export` based variables didn't always result in re-running the query that was using the variable. Unfortunately, these changes are a bit too aggressive. They're triggering a `refetch` when an `@client @export` based variable changes (and a few additional conditions are met), and while using the `ObservableQuery.refetch()` method does fix the original issue, it leads to a situation where the query being re-run is always fired over the network, even if the updated query could have been resolved from the cache. `ObservableQuery.refetch()` forces queries to use a network based fetch policy, which means if the query was originally fired with a `cache-first` policy (for example), and has matching data in the cache after taking into consideration the new variable, that data won't be used and instead an extra unnecessary network request will fire. This commit addresses the issue by leveraging `ObservableQuery.setVariables()` instead of automatically refetching. `setVariables` will attempt to resolve the updated query from the cache first, then only fire it over the network if required.
hwillson
added a commit
that referenced
this pull request
Feb 14, 2020
PR #4604 fixed an issue where changes made to `@client @export` based variables didn't always result in re-running the query that was using the variable. Unfortunately, these changes are a bit too aggressive. They're triggering a `refetch` when an `@client @export` based variable changes (and a few additional conditions are met), and while using the `ObservableQuery.refetch()` method does fix the original issue, it leads to a situation where the query being re-run is always fired over the network, even if the updated query could have been resolved from the cache. `ObservableQuery.refetch()` forces queries to use a network based fetch policy, which means if the query was originally fired with a `cache-first` policy (for example), and has matching data in the cache after taking into consideration the new variable, that data won't be used and instead an extra unnecessary network request will fire. This commit addresses the issue by leveraging `ObservableQuery.setVariables()` instead of automatically refetching. `setVariables` will attempt to resolve the updated query from the cache first, then only fire it over the network if required.
hwillson
added a commit
that referenced
this pull request
Feb 15, 2020
PR #4604 fixed an issue where changes made to `@client @export` based variables didn't always result in re-running the query that was using the variable. Unfortunately, these changes are a bit too aggressive. They're triggering a `refetch` when an `@client @export` based variable changes (and a few additional conditions are met), and while using the `ObservableQuery.refetch()` method does fix the original issue, it leads to a situation where the query being re-run is always fired over the network, even if the updated query could have been resolved from the cache. `ObservableQuery.refetch()` forces queries to use a network based fetch policy, which means if the query was originally fired with a `cache-first` policy (for example), and has matching data in the cache after taking into consideration the new variable, that data won't be used and instead an extra unnecessary network request will fire. This commit addresses the issue by leveraging `ObservableQuery.setVariables()` instead of automatically refetching. `setVariables` will attempt to resolve the updated query from the cache first, then only fire it over the network if required.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When using
client.watchQueryto setup anObservableagainst a query that's using@client @exportdirectives, cache changes could happen that lead to the value of the@client @exportvariable changing. When this happens, if the watched query is using a mixture of local and remote data, the watched query itself might become invalid, since the@client @exportvariable used to fetch the original data has now changed.To address this, this PR updates the
ObservableQueryobservers such that they first check to see if@client @exportdirectives are being used, and update any associated variables.ObservableQuerywill then perform arefetchusing the new variables if needed, otherwise it will let the observers continue with their normal behaviour.It's important to note that these changes introduce Promises into
ObservableQuerysubscription handling, which can impact the timing ofObservableQuerysubscriber object calls and/or callbacks. I haven't yet determined how much of an impact this could have in practice, but these changes did impact one existing test suite. I've modified the tests to accommodate these changes (as I believe that test did not represent a real usage scenario), but I think we'll need to consider the impact of these changes carefully before merging.Fixes #4530.