tighten QueryTuple to indicate observable fields may be absent #5935
Conversation
|
@benmosher: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
| @@ -84,13 +85,13 @@ export class QueryData<TData, TVariables> extends OperationData { | |||
| queryResult, | |||
| lazy = false, | |||
| }: { | |||
| queryResult: QueryResult<TData, TVariables>; | |||
| queryResult: QueryResult<TData, TVariables> | LazyQueryResult<TData, TVariables>; | |||
There was a problem hiding this comment.
I know this is redundant (since LazyQueryResult<D,V> already includes QueryResult<D,V>) but this felt like a reasonable duplication in the interest of clarity (since both lazy and non-lazy paths go through here).
| lazy?: boolean; | ||
| }) { | ||
| this.isMounted = true; | ||
|
|
||
| if (!lazy || this.runLazy) { | ||
| this.handleErrorOrCompleted(queryResult); | ||
| this.handleErrorOrCompleted(queryResult as QueryResult<TData, TVariables>); |
There was a problem hiding this comment.
!lazy || this.runLazy does properly enforce this type cast at runtime, AFAICT. but TS compiler doesn't know that.
| data: undefined; | ||
| } | ||
|
|
||
| type Impartial<T> = { |
There was a problem hiding this comment.
named as such as a complement to Partial<T>. I am totally fine with a less cute name if you have one in mind.
| export type LazyQueryResult<TData, TVariables> = | ||
| | UnexecutedLazyResult | ||
| | QueryResult<TData, TVariables>; | ||
|
|
||
| export type QueryTuple<TData, TVariables> = [ |
There was a problem hiding this comment.
FWIW: was tempted to rename this to LazyQueryTuple but I didn't want to rock the boat too much.
| @@ -82,7 +82,7 @@ export interface QueryResult<TData = any, TVariables = OperationVariables> | |||
| error?: ApolloError; | |||
| loading: boolean; | |||
| networkStatus: NetworkStatus; | |||
| called: boolean; | |||
| called: true; | |||
There was a problem hiding this comment.
assuming this field does not mean anything outside of lazy results, I believe this is correct now, as of the preceding commit.
There was a problem hiding this comment.
Hi,
I was wondering why you think this is correct.
By looking at the code I can see that called can definitely be false:
- initially here: https://github.com/apollographql/apollo-client/blob/main/src/react/hooks/useLazyQuery.ts#L35
- and again here: https://github.com/apollographql/apollo-client/blob/main/src/react/hooks/useLazyQuery.ts#L84
I spotted this strange type declaration because I'm getting an @typescript-eslint/no-unnecessary-condition warning on if (result.called) { ... The warning is correct but the type is not.
In my interpretation called should be false if the lazy initialization is not yet called. This how it seems to be implemented.
Is this perhaps deprecated? Did you intend to override called back to boolean for LazyQueryResult?
There was a problem hiding this comment.
The type of called was relaxed to boolean again in v3.5.8, thanks to #9328.
There was a problem hiding this comment.
it's been ages, but I think you're right that I meant to have LazyQueryResult set called: boolean. sorry for the bug!
Codecov Report
@@ Coverage Diff @@
## master #5935 +/- ##
=======================================
Coverage 94.59% 94.59%
=======================================
Files 91 91
Lines 3703 3703
Branches 908 875 -33
=======================================
Hits 3503 3503
Misses 177 177
Partials 23 23 Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thanks very much for working on this @benmosher! This looks great. 👍
fixes #5933
npm tandnpm run buildboth came back clean for me.Checklist: