Query: Fix data is undefined on error

[TLadd]

If the query fails on the first result, the query result data was being returned as undefined
Now this case will result in data being an empty object

Fixes #1977

[apollo-cla]

@TLadd: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[hwillson]

Thanks for working on this @TLadd! I agree with your assessment that the data result should be an empty object. Your changes look great, but I'm a little concerned that this is a breaking change. Some people have likely noticed that data is undefined when there is an error, and are leveraging that fact in their error handling logic. Rolling this change out could affect that since their !data checks will now fail. Hmmm - we'll need to think about this a bit more ... 🤔.

[TLadd]

Using !data to check for errors would be pretty brittle anyways, since it only happens if there isn't already a previous result. Certainly could be possible that there is code out there depending on it though.

[alamothe]

The typings say data is either TData or undefined - never empty object:

react-apollo/src/Query.tsx

Line 97 in 3985b47

data: TData | undefined;

So which one is it?

[hwillson]

Thanks for working on this @TLadd - I've added a note regarding the potential issues around data now always being an object. This is a good change though, so hopefully that's enough ... 🤞. LGTM!

[TLadd]

Thanks for getting it across the finish line!

[derekclair]

🥇

[danilobuerger]

@hwillson @TLadd this should be reverted, see #2424 . This PR promised:

instead of `data && data.user` you can just check `data.user`

instead it made things worse. Now a type guard is required to discriminate between TData and {}.

[tomitrescak]

Yes please, revert this change. It makes this VERY difficult to use in Typescript ;(

[hwillson]

@danilobuerger @tomitrescak Rolling back and we'll have a new release out this morning - thanks!

[kallaspriit]

So what ise the solution? Also the TypeScript example does not seem to work (any more) and it uses the old method of:

examples/typescript/src/Character.tsx

if (!data) return <div>no data</div>;

[hwillson]

@kallaspriit For now we'll roll this back, then we'll come up with another solution afterwards (we really didn't want breaking changes in this release). The examples are in the process of being updated and will be available shortly. We're also going to wire the example app tests into CI, to help keep them healthy.

[kallaspriit]

Wrote my comment seconds before seeing your reply. Sounds great :)

[fenok]

This should be re-reverted. #2424 can be fixed by replacing

data: TData | {};

with

data: Partial<TData>;

Which is essentially the same, but don't break types.

[tomitrescak]

@fenok that is not true. Those are fundamentally different concepts and would validate very differently. Your example also allows partial objects, while the former allows only complete objects or empty objects

[fenok]

Valid point, I was trapped in my partial results world. I also found that the idea of Partial has already been discussed and rejected. Sorry for the interruption 😅

[evenfrost]

So, what's the current status of it?

[lucasconstantino]

@evenfrost it was changed, but it's changed back from the previous change, which actually changed to keep things the same they were before the change, so that we don't really change what was changed before.

Edit: jokes apart, current state is:

data might be undefined if query errored and cache has no previous valid data.

[IanVS]

Thank you so much for clarifying, @lucasconstantino. I was getting really lost in trying to understand where things stand. 💫

Note, I just discovered that data can also be undefined when using components and setting skip={true} if there is no cache. This bit me because in the older hoc-based approach, the config.props function wasn't called at all, and turns out I was relying on that in one case. Just leaving a note here in case it's helpful for others.