Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[react-apollo] Update type for QueryRenderProps #2854

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

ganemone
Copy link
Contributor

@ganemone ganemone commented Oct 18, 2018

This improves the types for QueryRenderProps with respect to props.data. It correctly handles props.data being null, an empty object, or TData.

See #2172 (comment) for more context

@ganemone ganemone changed the title Update type for QueryRenderProps [react-apollo] Update type for QueryRenderProps Oct 18, 2018
Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 (Still need an approval from someone with authority obviously)

@ganemone
Copy link
Contributor Author

cc @wzrdzl @thymikee @rasmusprentow

@gantoine
Copy link
Member

Code-wise this LGTM, I'll wait to see if the people you pinged will weigh in before merging this.

@gantoine gantoine self-assigned this Oct 19, 2018
@gantoine gantoine added enhancement An addition to an existing component libdef Related to a library definition labels Oct 19, 2018
@vovacodes
Copy link
Contributor

LGTM! thanks for fixing it, it was annoying

@gantoine
Copy link
Member

Thanks everyone for contributions and/or feedback!

@gantoine gantoine merged commit f056b93 into flow-typed:master Oct 19, 2018
@jedwards1211
Copy link
Contributor

It correctly handles props.data being null

@ganemone just wanted to point out that void only accepts undefined, not null (not that it's causing me problems in this case)

1: const foo: void = null
                     ^ Cannot assign `null` to `foo` because null [1] is incompatible with undefined [2].

@jedwards1211
Copy link
Contributor

jedwards1211 commented Dec 11, 2018

If Apollo ever ends up getting rid of the empty object behavior, would you guys have any opposition to changing this to just TData | void?

I was using a utility function that normalizes the empty object to undefined, and although I can get rid of that utility function now, in some cases I have to perform more null checks now than I did before, which is slightly annoying.

@jedwards1211
Copy link
Contributor

Wish I had weighed in on this before merge, but my fault that I didn't.
It can get pesky to refactor the following code to add the null checks that are now necessary:

        updateAfterDelete={(cache: DataProxy, {data: rawData}: FetchResult<DestroyDeviceGroupMutationData>) => {
          const data = normalizeData(rawData)
          if (!data) return
          const {destroyDeviceGroup: {deviceGroupId}} = data

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An addition to an existing component libdef Related to a library definition
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants