Fix issues with queryOptions factory method types #8394
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit e464740. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
| : TType & { | ||
| [dataTagSymbol]: TValue | ||
| [dataTagErrorSymbol]: TError | ||
| } |
There was a problem hiding this comment.
Fixes this:
becomes this:
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8394 +/- ##
===========================================
+ Coverage 46.21% 63.03% +16.81%
===========================================
Files 198 134 -64
Lines 7509 4804 -2705
Branches 1711 1344 -367
===========================================
- Hits 3470 3028 -442
+ Misses 3664 1536 -2128
+ Partials 375 240 -135 |
can you try the PR preview builds? |
Thanks, that's a life-safer, the content of this PR has tRPC's codebase passing now (trpc/trpc#6290) albeit with one fix for correctness related to the DataTag change: |
Nick-Lucas
force-pushed
the
fixing-type-issues
branch
from
1030e64 to
2cc0950
Compare
| @@ -13,7 +13,7 @@ export type UndefinedInitialDataInfiniteOptions< | |||
| TQueryFnData, | |||
| TError = DefaultError, | |||
| TData = InfiniteData<TQueryFnData>, | |||
| TQueryKey extends QueryKey = QueryKey, | |||
| TQueryKey extends QueryKey = Array<any>, | |||
There was a problem hiding this comment.
my idea was to keep this to:
TQueryKey extends QueryKey = QueryKey,
and then just change what QueryKey is:
- export type QueryKey = ReadonlyArray<unknown>
+ export type QueryKey = Array<any>There was a problem hiding this comment.
Right! Sorry my bad, you did write that. Have tried it just to double check but no dice. The initial error about it being readonly is a symptom of the problem, but the full problem is the flipped order of assigning sub-type to super-type caused by the type being referenced in a callback.
The fix for this is always to provide your types correctly at the top layer so both directions of structural comparison can pass, and Tanstack Query has been masking this problem through query keys not always being respected on Client methods. This masking became really obvious with the new tRPC Client though because we can't type-anything without the DataTag flowing through
There was a problem hiding this comment.
Wait I made another mistake there keeping unknown, will come back
There was a problem hiding this comment.
Okay yep same answer as above:
Array<unknown> -> we get assignment error in callbacks because unknown can't be assigned to a known type
Array<any> -> assignment errors fixed, but we instead of get errors when assigning const/inline keys to query keys (can't assign readonly to mutable)
ReadonlyArray<any> -> we get assignment error in callbacks because they're assigning a readonly to a mutable (but this time in reverse)
Bonus: export type QueryKey = ReadonlyArray<any> | Array<any> also doesn't work, both elements of the union would need to pass checks in callbacks and as above the readonly one can't pass
There was a problem hiding this comment.
Only way to win here is to correctly type interfaces, which anybody using the standard methods in Tanstack Query will get for free, but anybody passing around query options would need to update their argument types as per the one failing test in this PR
There was a problem hiding this comment.
Pushed an alternate approach, it's back to where I started but with a clearer user story inspired by tRPC: #8394 (comment)
Nick-Lucas
force-pushed
the
fixing-type-issues
branch
from
2cc0950 to
0d57717
Compare
Nick-Lucas
force-pushed
the
fixing-type-issues
branch
3 times, most recently
from
9beffc5 to
1f7d0b2
Compare
| function somethingWithQueryOptions(options: UseQueryOptions<number>) { | ||
| function somethingWithQueryOptions<TQueryOpts extends AnyUseQueryOptions>( | ||
| options: TQueryOpts, | ||
| ) { |
There was a problem hiding this comment.
Hey @TkDodo so I think the other two approaches proved to be dead ends, and I'm also unsure about making major internal changes like those as they risk creating a new wave of breaking issues.
What I propose is we export types like AnyUseQueryOptions, and that way consumers wanting to create flexible re-usable methods can use a more correct form of generics like above (don't necessarily need the generics but for anything non-contrived it's probably useful). This is how tRPC and a few other projects solve this issue and it's rock solid
Changes are pushed
Nick-Lucas
force-pushed
the
fixing-type-issues
branch
from
d0151ee to
bc8908d
Compare
Nick-Lucas
force-pushed
the
fixing-type-issues
branch
from
bc8908d to
e464740
Compare
This tackles a few things we've spotted in tRPC:
tRPC build here: trpc/trpc#6290