-
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
[react-apollo] Misc. react-apollo improvements #2180
[react-apollo] Misc. react-apollo improvements #2180
Conversation
963d1ba
to
6fd326e
Compare
UpdateQueryOptions, | ||
FetchMoreQueryOptions, | ||
SubscribeToMoreOptions, |
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'm assuming you're already aware of this, but just to be sure, all of these imports from apollo-client are just resolving to any (both because this import is outside of declare module and because a definition doesn't currently exist).
Started on a PR to add a definition for apollo-client: #2172
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 did know that they resolved to any, but I wasn't aware that they had to be in declare module. It'll be interesting to see how much we can reuse when your PR is done!
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.
FAQ for reference: https://github.com/flowtype/flow-typed/wiki/FAQs#how-do-i-build-type-definitions-that-depend-on-other-definitions.
Appreciate your improvements to the libdef by the way!
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.
Thanks for the link!
6fd326e
to
5f1e5cb
Compare
client: ApolloClient | ||
} | ||
|
||
declare export type QueryRenderPropFunction<TData, TVariables> = (QueryRenderProps<TData, TVariables>) => React$Node | ||
|
||
declare export class Query<TData, TVariables> extends React$Component<{ | ||
declare export class Query<TData, TVariables, TSubscriptionData, TSubscriptionVariables> extends React$Component<{ |
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.
Would it make sense to add defaults for these?
At the moment, I'm extending the Query component and passing in the types, similar to how the apollo typescript docs suggest (https://www.apollographql.com/docs/react/recipes/static-typing.html#typing-components). This change would require going back and adding void, void for each of my Query components.
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.
Actually adding these was a mistake and they aren't used. I chose not to propagate these type variables because their usability is doubtable. The Query component doesn't "care" about those types, and that is also why the QueryRenderProps
doesn't have them.
5f1e5cb
to
2f9d63b
Compare
I'll wait for @TLadd to sign off. As part of this PR would you mind adding a |
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.
Waiting on approval.
I've added the |
898c43a
to
a858187
Compare
a858187
to
f77395a
Compare
@AndrewSouthpaw Looks good to me |
This appears to make flow unhappy:
|
Closes #2175
This enables us to annoate
subscribeToMore
with the following methodsimilar functionality has been added for
fetchMore
.