React-apollo: Breaking typescript change - definition of "data" on Query Result changed

[tomitrescak]

Since upgrade to newest version of react-apollo the data definition on QueryProps has changed from

export interface QueryResult<TData = any, TVariables = OperationVariables> extends ObservableQueryFields<TData, TVariables> {
...
data: TData | undefined;
}

to

export interface QueryResult<TData = any, TVariables = OperationVariables> extends ObservableQueryFields<TData, TVariables> {
...
data: TData | {};
}

This is a major breaking change as all projects with updated client do not compile and fail CI. What is the reason for this change? Will this be rectified or shall we start changing our code bases? Thanks for the wonderful work you are doing guys.

Version

• [email protected]

[danilobuerger]

This was introduced by #1983. The reason was that when using data, the undefined check can be omitted. Instead of doing data && data.user you can just check data.user. Full changelog:

• When a query failed on the first result, the query result data was being
  returned as undefined. This behavior has been changed so that data is
  returned as an empty object. This makes checking for data (e.g.
  instead of data && data.user you can just check data.user) and
  destructring (e.g. { data: { user } }) easier. Note: this could
  potentially hurt applications that are relying on a falsey check of data
  to see if any query errors have occurred. A better (and supported) way to
  check for errors is to use the result errors property.

@tomitrescak can you provide an example of what you were doing that now doesn't compile and fails?

[JakeSidSmith]

I had a look into this yesterday, and had a poke inside Query.test.tsx. The tests only have no issues because any test cases where the keys of the data are accessed use the Query class itself (rather than extending it), and default data to any.

I added the following (using some existing test data), and you can see that without casting the data to any, you cannot access its keys.

it('result type definition lets you access data keys', () => {
  const Component = () => (
    <AllPeopleQuery query={allPeopleQuery}>
      {(result) => {
        if (result.data && result.data.allPeople) {
          return null;
        }

        if (result.data && result.data!.allPeople) {
          return null;
        }

        const { allPeople } = result.data;

        return null;
      }}
    </AllPeopleQuery>
  );
});

[danilobuerger]

@JakeSidSmith thanks! So now a type guard would be required instead of a simple undefined check.

[JakeSidSmith]

Yes, it seems like a bit much though. This would have been easier with undefined.

Alternative solution (though personally I think TData | undefined makes sense).

data: TData | {[i: string]: undefined}

Which works with the above example.

[tomitrescak]

Yes, please. I think the undefined check is enough. Well supported by typescript. I'm doing exactly what @JakeSidSmith and that's exactly the place where my compilation is failing.

While your undefined check can be omitted during destructure, you are losing typesafety of the data!

you have to manually now recast, importing extra types ... all that funky jazz ;(

(result as MyResultType).data // bleh

[hwillson]

A breaking change was definitely not intended ☹️. I'll roll back to TData | undefined.

@JakeSidSmith Thanks for pointing that test issue out - I'll get that fixed. We're also putting other changes in place to prevent this from happening in the future. Thanks all - sorry for the headache!