React-apollo: onCompleted callback typedef is not destructuring friendly

[jozanza]

Intended outcome:
I'd like to have a destructuring-friendly type definition for component's onCompleted callback property.

Actual outcome:

I want to be able to destructure the onCompleted callback argument in a type-safe way:

<Query<{ foo: string }> query={GetFoo} onCompleted={({ foo }) => {}} />

This works at runtime, but TypeScript complains about the definition of the parameter passed to the onCompleted callback:

[ts] Type '{} | { foo: string; }' has no property 'foo' and no string index signature.

How to reproduce the issue:

Check out this CodeSandbox: https://codesandbox.io/s/lpjj2pom0z

Version

• [email protected]

[JoviDeCroock]

Normally the callback provided in onCompleted is wrapped in an object named data. This implies the result from your completed Query. Since this is handled by Apollo you should be ensured of the answer or am i overlooking something here?

[jozanza]

@JoviDeCroock If I were to create a schema and connect Apollo, foo would indeed be a string. The data is there working properly, so that's awesome. No runtime issue 👍

The problem is that onCompleted is defined as

(data: TData | {}) => void

So TypeScript thinks the data object passed to the callback is either TData, which I defined as { foo: string } or, for no reason I can infer, an unindexed empty object {}.

Since that latter case is unindexed and does not contain my foo prop, destructuring fails static typing.

Does that make sense?

From what I can tell, the data param passed to the callback should always be TData, so in my PR, I just changed the definition to

(data: TData) => void

[CarloPalinckx]

| {}

Is also a pretty bad developer experience for people running stricter typescript setups since there isn't really a decent typeguard (that im aware of) for that typescript can infer itself compared to something like null or undefined.

[jozanza]

@JoviDeCroock Is there anything you need me to change about this PR? Or can it be considered for merging?

[JoviDeCroock]

@jozanza I'm not a contributor or a member, just trying to help out where I can PR seems fine for me personally. I think the Apollo team is a bit swamped with the upcoming event.

[jozanza]

Ah sorry! Mybad.

[JoviDeCroock]

Made a small comment on your PR of something i just thought about :)

[andykenward]

For now, I define the typing for the argument directly to overcome this.

<Query<QueryEditCompany, QueryEditCompanyVariables>
  query={COMPANY}
  onCompleted={(data: QueryEditCompany) => {//do something}}
>