Improved type definitions for the observable query#3140
Improved type definitions for the observable query#3140hwillson merged 8 commits intoapollographql:masterfrom
Conversation
Generated by 🚫 dangerJS |
| */ | ||
| public setVariables( | ||
| variables: any, | ||
| variables?: TVariables, |
There was a problem hiding this comment.
Question: Is this supposed to be nullable. The type was any before so anything was allowed. If I make the variables nonnull here then I get typescript errors.
There was a problem hiding this comment.
hmm, I don't the the intention is for it to be nullable here. What issues did you run into?
| fetchMoreResult?: { [key: string]: any }; | ||
| variables: { [key: string]: any }; | ||
| fetchMoreResult?: TData; | ||
| variables?: TVariables; |
There was a problem hiding this comment.
Question: Is this supposed to be nullable? It wasn't before but typescript is giving me errors if I make this nonnull.
There was a problem hiding this comment.
but, yes variables can be null here
jbaxleyiii
left a comment
jbaxleyiii
left a comment
There was a problem hiding this comment.
@excitement-engineer lets figure out the setVariables bit, but otherwise YAYAYAYA
| fetchMoreResult?: { [key: string]: any }; | ||
| variables: { [key: string]: any }; | ||
| fetchMoreResult?: TData; | ||
| variables?: TVariables; |
| fetchMoreResult?: { [key: string]: any }; | ||
| variables: { [key: string]: any }; | ||
| fetchMoreResult?: TData; | ||
| variables?: TVariables; |
There was a problem hiding this comment.
but, yes variables can be null here
| */ | ||
| public setVariables( | ||
| variables: any, | ||
| variables?: TVariables, |
There was a problem hiding this comment.
hmm, I don't the the intention is for it to be nullable here. What issues did you run into?
|
oh @excitement-engineer could you add a changelog here as well? Thanks! |
| const doc = getDirectivesFromDocument([{ name: 'client' }], query, true); | ||
| expect(print(doc)).toBe(print(expected)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
This was missing in apollo-client. This was causing the prettier pre-commit hook to fail...
| ): Promise<ApolloQueryResult<TData>> { | ||
| // since setVariables restarts the subscription, we reset the tornDown status | ||
| this.isTornDown = false; | ||
|
|
There was a problem hiding this comment.
In the line below here we are doing:
const newVariables = variables ? variables : this.variables;So I am wondering if it isn't possible to make the type variables?: TVariables?
|
|
||
| return this.setVariables( | ||
| this.options.variables, | ||
| this.options.variables as TVariables, |
There was a problem hiding this comment.
This is the issue I ran into with setVariables having a non-null TVariables type. this.options.variables is nullable here so I had to make an explicit cast to get typescript to work
hwillson
left a comment
hwillson
left a comment
There was a problem hiding this comment.
Thanks for all of your work on this @excitement-engineer! We should be all set here, so LGTM!
ghost
mentioned this pull request
4 participants
The Query component in react-apollo improved upon the typescript definitions provided by apollo-client.
I am now porting these improvements back to apollo-client so that all users can benefit from these improvements and so that react-apollo can simply import the type definitions directly from apollo-client.
This PR makes introduce generic types for the data
TDataand variablesTVariablesin theObservableQueryclass.