-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Apollo Client Libdef (also add to react-apollo) #2172
Conversation
4b99f47
to
1727d5e
Compare
Are the checklist items TODOs, or are they done? |
They are TODOs. Sorry for the confusion. |
@TLadd let me know when you're ready for a review and I'll gladly abide. Regarding the imports; considering the poor support for inter libdef dependencies (ref #16) I'm really starting to think that the imports should be removed and the types duplicated. I'm planning on removing all the imports from react-apollo and clean up duplicate declarations during the next week. |
Yeah, from #1857 (comment), it appears that even if the imports worked, the tests wouldn't. Just duplicating the types react-apollo needs is probably the path of least resistance at the moment. I think as long as we distinguish clearly in the file where the types are actually coming from (this section is actually react-apollo, this section is types from apollo-client, etc) maintenance won't be that bad. Sorry this has been stagnating a bit; I'll have some time over the weekend to make a push and should get it to a mergeable state. |
Yes, that sounds like a good plan!
…On Thu, May 10, 2018, 14:31 Thomas Ladd ***@***.***> wrote:
Yeah, from #1857 (comment)
<#1857 (comment)>,
it appears that even if the imports worked, the tests wouldn't. Just
duplicating the types react-apollo needs is probably the path of least
resistance at the moment. I think as long as we distinguish clearly in the
file where the types are actually coming from (this section is actually
react-apollo, this section is types from apollo-client, etc) maintenance
won't be that bad.
Sorry this has been stagnating a bit; I'll have some time over the weekend
to make a push and should get it to a mergeable state.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#2172 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADVCy_r3FxbRnUgZy4OXVRTqiZUAVHNTks5txDMTgaJpZM4TzuC7>
.
|
…apollo Add more tests for Query and Mutation components Add tests for ApolloProvider and ApolloConsumer
@budde377 @gantoine think this is ready for a look. I copied the types from apollo-client into react-apollo as well. Sorry for the diff size. I'm not sure how fun keeping the types in apollo-client and react-apollo in sync will be. I'm hoping it's not too bad, since it is just a matter of copy/pasting them over. If it gets to be too much of a pain, I'm not opposed to abandoning apollo-client and just maintain everything in react-apollo. At least in my projects, the only place I'm using apollo-client directly is to initialize the client (hence the tests checking ApolloClient's initialization). So not a huge deal, but I think they're worth having around if it's easy. Added some tests focused around the Query and Mutation props I'm using. Also some stylistic changes in the PR caused by prettier, just using the default config. I'm fine shifting the style some if there are complaints, but would prefer it be prettier-able. Pulled both these libdefs into the project I'm using react-apollo in and seems to be behaving correctly. Definitely still room for improvement in places, but hopefully a good step. |
Nice! I can prob. do a review monday afternoon (CET) :) |
/** End From graphql */ | ||
|
||
declare export interface ApolloProviderProps<TCache> { | ||
client: any; // ApolloClient<TCache>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any or apollo client?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The trouble with using ApolloClient is that it's a class and it's nominally typed. So even though ApolloClient is the same in both the apollo-client and react-apollo libdefs, they are different types. We could maybe make it work with an interface in react-apollo, but didn't think it was worth it to diverge from what is defined in apollo-client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see... Let's leave it as is for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! I do not mind your style changes and love the extensions to the test suite. This libdef also works in my projects. I'll approve 👍
@gantoine Looks like this is ready for you whenever you get the time. Thanks! |
@TLadd using Are you sure Apollo can pass an empty object? If so would you be okay with us changing this to |
Example of the problem (Try flow link): @flow
type Data = {foo: number} | {||}
function getFoo(data: Data): ?number {
return data.foo
}
|
… of flow react-apollo passes an empty object before data is loaded; data will never be undefined. However, using `| {||}` just isn't going to work with recent versions of flow (see flow-typed#2172 (comment)) I think `$Shape<TData>` is the only viable option here.
I believe this has always been the case (you still get an error if you select older versions of flow as well). I'm typically checking that values are present when I'm accessing them, and it removes the type errors. This works for instance:
Having said that, it may be reasonable to change data to just be |
@TLadd actually I read the react-apollo source, it never passes an undefined value, not anymore at least. I didn't realize flow has that obscure exception for checking if an object is exact empty or not. Thanks for pointing that out. Quite subtle because |
@jedwards1211 There is actually still one case where undefined gets passed in: apollographql/react-apollo#1977 Typescript actually types data as The only trouble with doing that is suddenly
would typecheck, since the if check would refine it to type TData, but it might actually be the empty object. |
Ah right. Argh... |
I think we can actually fix this using I will submit a PR. |
@ganemone |
@TLadd I don't think you were the one to introduce this, but can a |
It's the same in the typescript defs: https://github.com/apollographql/react-apollo/blob/master/src/Mutation.tsx#L62 Not sure; would have to look through the source to see how/if this can happen. |
No worries, just wanted to check if you knew off the top of your head, thanks! |
It seems that since the react-apollo libdef is importing types from apollo-client (which doesn't have types), even if I include this apollo-client libdef in my project, types that react-apollo imports end up getting clobbered to any. Either need to
Actually using the types is definitely the endgame, but might be a large effort that could be split to another PR.