Fix cache-and-network cachePolicy queries never dispatching APOLLO_QUERY_RESULT_CLIENT

[BlooJeans]

Previously: If you ran a cache-and-network query, it would read the query from the store and then never use it. This effectively made cache-and-network queries function as network-only

TODO:

• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change
• Add your name and email to the AUTHORS file (optional)

[helfer]

Thanks @BlooJeans ! Could you add a test for this?

[BlooJeans]

@helfer I found a test

apollo-client/test/client.ts

Line 1194 in ec9437f

it('fetches from cache first, then network', (done) => {

, but I'm not familiar enough with apollo internals to know if it's testing something unrelated, if it's insufficient, or if it's broken.

It seems that the above test is checking the number of times the observableQuery fires, but I don't know the relation between observables and redux actions.

Mind pointing me in the right direction?

[helfer]

@BlooJeans I'm pretty sure that test is working, so maybe the real question is: how did you determine that it's not working? Was the only problem that it didn't dispatch the redux action? Either a reproduction or a test that fails without this PR would be great. The test should look pretty much like the one you linked to.

[BlooJeans]

Sorry for the wall of text:

I determined it's currently fault because 'cache-and-network' claims to return the cached result, if available, and then a network request, and I wasn't noticing that in our app. I searched through the apollo repo to where apollo potentially dispatches the APOLLO_QUERY_RESULT_CLIENT and stepped through the code and realized that the current logic will never emit the APOLLO_QUERY_RESULT_CLIENT; it will always only call APOLLO_QUERY_RESULT when the network call returns.

If you glance over the logic (not too complicated):

      const { isMissing, result } = diffQueryAgainstStore({
        query: queryDoc,
        store: this.reduxRootSelector(this.store.getState()).data,
        variables,
        returnPartialData: true,
        config: this.reducerConfig,
      });

      // If we're in here, only fetch if we have missing fields
      needToFetch = isMissing || fetchPolicy === 'cache-and-network';

      storeResult = result;
    }

My debugging was not a refetch, nor was it network-only, so we get inside this if block. We check the cache, and my data was successfully retrieved (in full). So far so good.
But then the problems start:

needToFetch = isMissing || fetchPolicy === 'cache-and-network';

This will set needToFetch to true

Two lines later:

const shouldFetch = needToFetch && fetchPolicy !== 'cache-only';

Also sets shouldFetch to true

A few lines later: this is the faulty logic imo. Dispatching the data found in the cache shouldn't only happen only when shouldFetch is false.

    const shouldUseCache = !shouldFetch; // current logic
    // const shouldUseCache = !shouldFetch || fetchPolicy === 'cache-and-network'; // XXX my change
    if (shouldUseCache) {
      this.store.dispatch({
        type: 'APOLLO_QUERY_RESULT_CLIENT',
        result: { data: storeResult },
        variables,
        document: queryDoc,
        complete: !shouldFetch, // XXX side note: without my change, this will always be true, because it's equivalent to the expression in the if()
        queryId,
        requestId,
      });
    }

Again, I'm not too familiar with the internals, so maybe cache-and-network is working as intended. Assuming my analysis shows that this is a bug and my fix solves it, I'm happy to write a test to cover it, but I'm not sure what the current test is covering (i.e. the relationship between redux and the query observables)

[helfer]

Thanks @BlooJeans! I agree with you that it should dispatch the QUERY_RESULT_CLIENT action, so I'm going to merge the PR 🙂

[grifotv]

What is the difference between cachePolicy and fetchPolicy?

[BlooJeans]

@grifotv there's only a fetchPolicy, I mistakingly called it cachePolicy here

[grifotv]

@BlooJeans I found this same mistake here https://github.com/apollographql/apollo-client/blob/master/src/core/QueryManager.ts#L428 and here https://github.com/apollographql/apollo-client/blob/master/test/ObservableQuery.ts#L262