Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Query result proposal: use actual data or {} to enable easier destructuring #1686

Closed
JonathanZWhite opened this issue Feb 17, 2018 · 10 comments

Comments

@JonathanZWhite
Copy link

JonathanZWhite commented Feb 17, 2018

In the Query Component, if the query is still loading, the data variable will be undefined. This behavior is due to the following line:

data: isDataFilled(data) ? data : undefined

What do you think of making data {} instead of undefined? This way, it will be possible to do destructuring of data like so:

<Query query={HEROS_QUERY}>
  {({ loading, data: { heroes } }) => {
    if (loading) {
      return null;
    }

    return (...);
  }}
</Query>

Currently, this will throw Uncaught TypeError: Cannot read property 'heroes' of undefined. With the old query HOC pattern, this was not an issue (from the docs):

const ProfileWithData = graphql(CurrentUserForLayout, {
  props: ({ ownProps, data: { loading, currentUser, refetch } }) => ({
    userLoading: loading,
    user: currentUser,
    refetchUser: refetch,
  }),
})(Profile);

Since loading was moved out of data in the new query component, I can also see why it might make more sense to keep data as undefined when loading is true. Just wanted to get some opinions. 🎉

Version
react-apollo@2.1.0-beta.2

@derek-duncan
Copy link

I think this was resolved in #1635 issue report 👍

@blasterpistol
Copy link

I think with version 2.1.0-beta.3 you need to change the typings from result.data: TData | undefined to result.data: TData since data now is always an empty object.

@alamothe
Copy link

I'm using 2.1.8 and typings say TData or undefined - never empty object:

data: TData | undefined;

However, in runtime data is {}. Can you please confirm this is an error with typings? Is there an open issue for the fix?

@rosskevin
Copy link
Contributor

At one point, there was an empty object. It is now TData | undefined. Related #1917

@alamothe
Copy link

@rosskevin I think it's the other way round.

@rosskevin rosskevin reopened this Dec 27, 2018
@rosskevin
Copy link
Contributor

Related #1917

@rosskevin rosskevin changed the title [2.1.0-beta.2] Query component feedback: data undefined versus empty object QueryResult data undefined versus empty object Dec 27, 2018
@rosskevin rosskevin changed the title QueryResult data undefined versus empty object Query result proposal: use actual data or {} to enable easier destructuring Dec 27, 2018
@rosskevin
Copy link
Contributor

@alamothe at one point it was returning data | {} and types changed to use Partial, this was done in #2313 and reverted in #2423

As mentioned in #2423 (comment), the biggest problem with using data || {} for the result, while true it would enable easy destructuring, it makes someone then inspect the result to see if there is actual data.

For example (as that was the case some time ago), I needed all of the below to determine if I needed to render the LoadingComponent or the actual component rendering the data:

      <Query {...rest}>
        {result => {
          if (result.error) {
            // error handling code
          } else if (result.loading || Object.keys(result.data).length < 1) {
            // could be an empty object at this point because {}
            return LoadingComponent ? <LoadingComponent /> : null
          } else if (result.data) {
            // The above code guarantees that at this point it should be our real data
            const data: TData = result.data as TData
            return children({ client: result.client, data })
          } else {
            throw new Error('Unexpected fall through, check the log.')
          }
        }}
      </Query>

While this is easier for destructuring, as shown above, it requires more logic to determine if you have actual data to render.

@alamothe
Copy link

I absolutely agree with your rationale, and prefer undefined myself. It would be great if it returned to undefined, I will check.

But as far as I remember, it was undefined way back, then switched to {}. The changes you refer to are only about TypeScript typings (which had misadventures of their own), and not the actual logic.

If it's reverted to undefined, this must be a very recent change.

@hwillson
Copy link
Member

hwillson commented Sep 6, 2019

React Apollo has been refactored to use React Hooks behind the scenes for everything, which means a lot has changed since this issue was opened (and a lot of outstanding issues have been resolved). We'll close this issue off, but please let us know if you're still encountering this problem using React Apollo >= 3.1.0. Thanks!

@hwillson hwillson closed this as completed Sep 6, 2019
@ralvs
Copy link

ralvs commented Oct 2, 2019

Guys, I'm receiving the error above when I update above @apollo/react-hooks@3.0.1
Last test with @apollo/react-hooks@3.1.2 and @apollo/react-ssr@3.1.2
Version 3.0.1 is the last that works for me.

TypeError: Cannot destructure property `tenant` of 'undefined' or 'null'.

My code

const {error, data: { tenant } } = useQuery(CURRENT_TENANT_QUERY);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants