[3.0.0-rc.12] empty object returned when calling refetch on watchQuery observable with notifyOnNetworkStatusChange and no-cache policy

[wassim-k]

Intended outcome:
Receive loading status with current data object when calling refetch on client.watchQuery with notifyOnNetworkStatusChange: true and fetchPolicy: no-cache

Actual outcome:
After refetch, an empty object is received.

● bug › should not return an empty object when refetching with (fetchPolicy: no-cache, notifyOnNetworkStatusChange: true)

    expect(received).toMatchObject(expected)

    - Expected  - 5
    + Received  + 1

      Object {
    -   "data": Object {
    -     "test": Object {
    -       "text": "test",
    -     },
    -   },
    +   "data": Object {},
        "loading": true,
      }

      41 |           break;
      42 |         case 2:
    > 43 |           expect(result).toMatchObject({ loading: true, data: expected });
         |                          ^
      44 |           break;
      45 |         case 3:
      46 |           expect(result).toMatchObject({ loading: false, data: expected });

How to reproduce the issue:

import { ApolloClient, ApolloLink, gql, InMemoryCache, Observable } from '@apollo/client/core';

const delay = (ms: number) => new Promise(resolve => setTimeout(resolve, ms));

describe('bug', () => {
  it('should not return an empty object when refetching with (fetchPolicy: no-cache, notifyOnNetworkStatusChange: true)', done => {
    const client = new ApolloClient({
      cache: new InMemoryCache(),
      link: new ApolloLink(operation => {
        return new Observable(subscriber => {
          delay(50).then(() => {
            subscriber.next({
              data: {
                test: {
                  text: 'test'
                }
              }
            });
            subscriber.complete();
          });
        });
      })
    });

    const query = client.watchQuery({
      query: gql`
        query test {
          test { text }
        }`,
      fetchPolicy: 'no-cache',
      notifyOnNetworkStatusChange: true
    });

    let count = 0;
    const expected = { test: { text: 'test' } };

    query.subscribe(result => {
      switch (++count) {
        case 1:
          expect(result).toMatchObject({ loading: false, data: expected });
          break;
        case 2:
          expect(result).toMatchObject({ loading: true, data: expected });
          break;
        case 3:
          expect(result).toMatchObject({ loading: false, data: expected });
          done();
          break;
      }
    });

    delay(100).then(() => query.refetch());
  });
});

Versions
@apollo/client: >=3.0.0-rc.0 => 3.0.0-rc.12

[benjamn]

Thanks for the detailed issue report!

As far as I can tell, you're getting what you asked for: notifyOnNetworkStatusChange: true causes a loading state to be inserted before the refetch happens (with result.networkStatus === NetworkStatus.refetch), and such loading states provide the latest result.data from the cache. In this case, you're not writing any data to the cache (because of the no-cache fetch policy), so result.data is empty. We could discuss whether result.data should be undefined or an empty object, but that doesn't seem to be the gist of your complaint.

If you don't have any use for the loading state, I would advise against setting notifyOnNetworkStatusChange to true (it's false by default), or at least adjust your code to check result.loading before using result.data.

In general, redelivering previous data for the sake of providing some data instead of none was a short-sighted hack in Apollo Client 2.x that caused more/worse problems than it solved. The old data doesn't necessarily have anything to do with what will be refetched, so it doesn't make sense to include it in the loading state. See #6566 for a similar (React-related) issue.

[wassim-k]

Thanks for the explanation, the reasoning makes sense.
I still think there's a bug here that needs to be fixed, because the type definition for data is TData | undefined
So typescript will compile the following code successfully:

if(result.data !== undefined) {
  // result.data type is narrowed to TData
  const textLength = result.data.text.length; // This line will throw a error at runtime
  ...
}

So I believe that data should be undefined instead of an empty object if no data was found in the cache.

Btw, let me know if you'd like me to raise a PR with the fix, I'd be happy to do so?

[wassim-k]

If I may be so bold and suggest that updating mergeDeepArray function in mergeDeep.ts to the following could be the fix:

export function mergeDeepArray<T>(sources: T[]): T {
  const merger = new DeepMerger();
  return sources.reduce(
    (acc, source) => acc ? merger.merge(acc, source) : source,
    undefined
  );
}

I believe it's the first line in the function let target = sources[0] || ({} as T); that falsely asserts the object's type.

I don't know what other unintended consequences this change may have but I hope I saved you a couple of precious minutes of investigating the problem, I know how busy you are.

[hwillson]

A lot of the Apollo Client internals have changed since v3 was launched. We recommend trying a more modern version of @apollo/client. Let us know if you're still encountering this issue. Thanks!