setVariables doesn't notify subscribers when cache hit

[szechyjs]

Intended outcome: Calling setVariables() on a ObservableQuery should notify subscribers of a data change. This works as expected if the query is not cached and results in a network request.

Actual outcome: If a query has been cached setVariables() does not notify its subscribers of the data change. From tracing the code it appears the correct data is pulled from the cache and set as the query result, however subscribers are never notified of the result like they are when a network query occurs.

How to reproduce the issue:

app.module.ts

constructor(apollo: Apollo, httpLink: HttpLink) {
  apollo.create({
    link: httpLink.create({}),
    cache: new InMemoryCache()
  });
}

data.service.ts

const TestQuery = gql`
  query TestQuery($name: String!) {
    users(name: $name) {
      name
      userid
    }
  }
`;

constructor(private apollo: Apollo) {
  this.queryRef = this.apollo.watchQuery<QueryResponse>({
    query: TestQuery,
    variables: {
      name: ""
    }
  });
}

getUsers() {
  return this.queryRef.valueChanges;
}

nameUpdate(name) {
  this.queryRef.setVariables({
    name: name
  });
}

view.component.ts

ngOnInit() {
  this.dataService.getUsers().subscribe({data}) => {
    this.users = data.users;
  });
}

// These are tied to user actions, but for demonstrating the issue, calling directly.
this.dataService.nameUpdate('bob');  // Performs new network query and updates observable
this.dataService.nameUpdate('john'); // Performs new network query and updates observable
this.dataService.nameUpdate('bob'); // Queries the cache and DOES NOT update observable

Versions

• apollo-client@2.1.0
• apollo-cache@1.0.2
• apollo-cache-inmemory@1.1.3
• apollo-angular@1.0.0

[s-panferov]

I can confirm this behavior, we ran into this too. And it seems to be a bug.

[s-panferov]

The same for refetch, it does not call .next() too

[sambaptista]

+1

Here is test implementation example where we request dataOne, then dataTwo and again dataOne.

In this case, a timeout is fired because handleCount variable never hits stage 4 and 5.

diff --git a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
index fca78f3d..7e42b736 100644
--- a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
+++ b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
@@ -679,6 +679,10 @@ describe('ObservableQuery', () => {
           request: { query, variables: differentVariables },
           result: { data: dataTwo },
         },
+        {
+          request: { query, variables: variables },
+          result: { data: dataOne },
+        },
       );

       subscribeAndCount(done, observable, (handleCount, result) => {
@@ -691,6 +695,12 @@ describe('ObservableQuery', () => {
         } else if (handleCount === 3) {
           expect(result.loading).toBe(false);
           expect(result.data).toEqual(dataTwo);
+        } else if (handleCount === 4) {
+          expect(result.loading).toBe(false);
+          expect(result.data).toEqual(dataTwo);
+        } else if (handleCount === 5) {
+           @expect(result.loading).toBe(true);
+          expect(result.data).toEqual(dataOne);
           done();
         }
       });

[hwillson]

Hi all - sorry for the delay in responding to this.

TL;DR

setVariables is working as designed.

Background

In a nutshell, watchQuery was designed to address the following:

... suppose you call watchQuery on a GraphQL query that fetches an person's first name and last name and this person has a particular object identifer, provided by dataIdFromObject. Later, a different query fetches that same person's first and last name and his/her first name has now changed. Then, any observers associated with the results of the first query will be updated with a new result object.

It does this by watching for changes made in the Apollo Client store. The example listed in https://github.com/apollographql/apollo-client/issues/2712#issue-281085838 is using watchQuery a bit differently than the above. It's not only expecting the ObservableQuery to be notified of store changes (ie. when a new fetch is run with new variables, and new data is added to the store), but it's also expecting to be notified when there are no changes made to the store (ie. using setVariables with variables that have already been used before, and have a matching entry in the store). setVariables doesn't notify subscribers when an existing match is found in the store, as nothing new has been added/updated in the store. It's assuming that whatever is watching for store data changes, really only wants to be notified when there are actual store data changes. So another way of looking at this is that watchQuery is intended to be used to watch for store changes, whereas the example in https://github.com/apollographql/apollo-client/issues/2712#issue-281085838 is expecting watchQuery to be usable for watching for query result changes (which may or may not come from the store, might have changed, might not have changed, etc.).

What has definitely added to the confusion here, is the fact that setVariables was not really intended to be part of apollo-client's public API. The fact that it's exposed in the docs is kinda a bug; it's publicly accessible to make it easier for developers who are working on Apollo Client integration packages, that reuse/extend existing AC functionality. Actually, most of the ObservableQuery functions listed in the API docs are not really meant to be used directly - this definitely needs to be fixed (:hwillson ties string around finger:).

ObservableQuery has a solution for handling situations like https://github.com/apollographql/apollo-client/issues/2712#issue-281085838, that is intended to be public - ObservableQuery.refetch. The idea behind refetch is that if your variables have changed and you want to fetch data using those variables, you can pass them into refetch(variables), have your data fetched, then saved back in the store, which then means your observers will be notified of the newly updated store data. Yes, refectch has one main drawback - it will always make a network request. So if you're refetching with variables that are already associated with data saved in the store, you'll be ignoring the store and refetching anyways. This can be worked around however by checking the cache first to see if a match exists, before firing the refetch.

So long story longer, setVariables is actually working as designed. We're definitely open to changing its behavior, but this is something that would need to be planned for a future major release. I'll add the discussion tag here to keep this issue open, and will be tracking all comments/suggestions/complaints/etc. Thanks all!

[PowerKiKi]

watchQuery is intended to be used to watch for store changes

This is a very surprising revelation as I was instinctively expecting to watch result changes, not store changes. And the docs in watchQuery clearly match my expectations:

This watches the results of the query according to the options specified and returns an ObservableQuery. We can subscribe to this ObservableQuery and receive updated results through a GraphQL observer.

Can you guarantee that if we use refetch() with new (or same) variables, but it just so happens that the result coming from network will be exactly the same as the one in cache, then the next will still be called ? even if the result is identical to whatever is in cache ?

So far, I would suggest the following:

1. Immediately improve the documentation
   • watchQuery should clearly state it watches the store, not the result
   • setVariables should clearly state it meant for internal uses only and should never be used. Do you have a established way to mark API as internal ?
   • refetch should have some minimal docs mentioning its primary usage (to "replace" setVariables usage)
2. Keep the discussion going for potential behavior changes in next major version

If you agree on the principle I can provide a PR for 1.

[PowerKiKi]

About the potential behavior changes... in what kind of use-cases would it be preferable to watch the store, instead of watching the result for a public API ? From the consumer point of view it seems that the difference between store and result is very thin if not non-existent.

If there aren't any strong use-cases for a public API to watch the store, then I'd say the behavior should change to always watch the results instead. Making the store even more transparent for the consumer for most common use-cases.

[sambaptista]

@hwillson Thx for feedback. I'm observing a behavior I can't say if it matches the design you explain.

What does determine if the store has change ? If I refetch from the server an item that is identical to the one that is on the store, has the store changed ?

Here is the context of my question : I have two actions with id 123 and 456. I use fetchPolicy cache-and-network.

On first fetch, I watch the query with id 123.

const queryRef = this.apollo.watchQuery({
    query: actionsQuery,
    variables: {id: 123},
});

queryRef.subscribe(result => console.log(result));

This cause networkStatus outputs successively

• {data: undefined, loading: true, networkStatus: 1, stale: true}
• {data: {id: 123}, loading: false, networkStatus: 7, stale: false}

With successive calls to setVariables({id: 123}) nothing happens, and that seems to match your explanations as the store did not change.

Now I execute with second id queryRef.setVariables({id: 456}).

A query is sent to the server and the console.log (valueChanges) outputs successively :

• {data: {id: 123}, loading: true, networkStatus: 2, stale: false}
• {data: {id: 123}, loading: true, networkStatus: 1, stale: false}
• {data: {id: 456}, loading: false, networkStatus: 7, stale: false}

The queryRef.valueChanges has been updated, but on networkStatus 2 and 1 we received old data. Question : is the old data here by design ? What's the point of it here ? It can cause the controllers to do some stuff with out of sync data.

At this point, the actions id:123 and id:456 are both on the store.

setVariables() has different interpretation of the changes in the store following bellow conditions :

• If I call setVariables() successively with same id, the store is not considered as changed and queryRef.valueChanges is not updated
• If I call setVariables alternating the ids, the store is considered as changed and the queryRef.valueChanges is updated.

Whether we alternate or not id when calling multiple times setVariables(), the store don't really change imho. The actions are already there. And so on queryRef.valueChanges should not be updated. Its seems to be the opposite of what you describe. Is it by design too ?

[PowerKiKi]

@hwillson I created #3692 to address the documentation issue, please have a look and let me know where I should add the CHANGELOG related to that.

@s-panferov, contrary to what you said, I can confirm that refetch() with policy cache-and-network will correctly notify subscribers even if result is coming from cache as demonstrated in that PR.

@sambaptista I agree that receiving old data does not qualify as a cache change. And it adds to the confusion. So when @hwillson said:

watchQuery is intended to be used to watch for store changes

It is incomplete and it should instead read:

watchQuery is intended to be used to watch for store changes, network status changes and loading status changes, but not result changes

It honestly feels awkward to write that watchQuery is not intended to watch for result changes but several other things, but that is the current situation. IMHO it feels like the behavior of watchQuery was not well-thought from the start from the API user point of view and is instead the result of a organic evolution.

[hwillson]

To help provide a more clear separation between feature requests / discussions and bugs, and to help clean up the feature request / discussion backlog, Apollo Client feature requests / discussions are now being managed under the https://github.com/apollographql/apollo-feature-requests repository.

Migrated to: https://github.com/apollographql/apollo-feature-requests/issues/25

[PowerKiKi]

Migrated to https://github.com/apollographql/apollo-feature-requests/issues/25, because the current situation is confusing at best or buggy at worst.