Improved type definitions for the observable query #3140
Conversation
Generated by 🚫 dangerJS |
| @@ -421,17 +430,14 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> { | |||
| * | |||
| */ | |||
| 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.
@excitement-engineer what errors did you get?
There was a problem hiding this comment.
but, yes variables can be null here
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; |
There was a problem hiding this comment.
@excitement-engineer what errors did you get?
| 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
| @@ -421,17 +430,14 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> { | |||
| * | |||
| */ | |||
| 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! |
| @@ -982,4 +982,5 @@ describe('getDirectivesFromDocument', () => { | |||
| 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...
| tryFetch: boolean = false, | ||
| fetchResults = true, | ||
| ): Promise<ApolloQueryResult<T>> { | ||
| ): 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?
| @@ -399,7 +408,7 @@ export class ObservableQuery<T> extends Observable<ApolloQueryResult<T>> { | |||
| false; | |||
|
|
|||
| 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
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!
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.