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

Use Partial<TData> rather than TData | {} #2313

Merged
merged 3 commits into from
Sep 20, 2018

Conversation

tgriesser
Copy link
Contributor

This is related to the changes in #1983 - I think Partial<TData> better captures the intent of the change and plays much nicer with TypeScript, intellisense autocompletion, etc.

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

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

Great idea @tgriesser - thanks very much!

@hwillson hwillson merged commit 2f15d9f into apollographql:master Sep 20, 2018
@donataswix
Copy link

Not sure why Partial<TData> was approved. This basically overrides whatever people used as their (carefully crafted) TData concrete type and is definitely not equivalent to TData | {}.

@rosskevin
Copy link
Contributor

I agree with @donataswix - use of Partial makes all properties optional and is not correct.

@hwillson we should get a patch out asap as this is a breaking change to all users who are strongly typed, I'll put up the PR now.

rosskevin added a commit to rosskevin/react-apollo that referenced this pull request Sep 26, 2018
hwillson pushed a commit that referenced this pull request Sep 26, 2018
@tgriesser
Copy link
Contributor Author

Yeah, I think main issue here was the change to TData | {} in #1983 was breaking in the first place, this was just a band-aid on that from a typing perspective since {} is effectively an any in TS.

Keeping TData | undefined and a boolean flag as to whether data has been returned (I believe I saw this mentioned elsewhere) feels like it would be the best solution here.

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

Successfully merging this pull request may close these issues.

4 participants