-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Fix type errors when using fully-typed inputs like QueryFilters<Data, Error>, and test all QueryClient methods to detect similar issues #8375
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit bf7f163. 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. |
2440ae9
to
d5bf673
Compare
75dfaba
to
26154ca
Compare
90f6a5d
to
8804138
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8375 +/- ##
===========================================
+ Coverage 46.12% 62.91% +16.79%
===========================================
Files 200 136 -64
Lines 7504 4797 -2707
Branches 1713 1344 -369
===========================================
- Hits 3461 3018 -443
+ Misses 3668 1539 -2129
+ Partials 375 240 -135 |
Hey @TkDodo I don't want to rush this one in as this changeset is wide-reaching. I experienced quite a few potential breaking changes I had to work around while developing this, so want to be really sure I'm not going to cause problems. The described issues on 5.62.0 should not impact current usage of RQ, but if anybody wants to use the |
@@ -36,9 +35,7 @@ export interface QueryFilters< | |||
/** | |||
* Include queries matching this query key | |||
*/ | |||
queryKey?: unknown extends TQueryFnData | |||
? QueryKey | |||
: QueryKey & DataTag<unknown, TQueryFnData, TError> |
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.
This was the wrong idea, any fully-typed usage will define the TQueryKey and pass it in, and this approach was proving unreliable when I added richer tests.
…h supports all cases
Okay simplified the changeset quite a lot now, instead of adding overloads it's looking a lot more like it looks on other methods which took generics, and the new tests & existing builds seem happy |
After adding type parameters to QueryFilters, some methods are proving to be incompatible with fully-typed usage
This PR adds a complete suite of fully-typed calls to all QueryClient methods, and method overloads which accept these inputs.
The core issue is that while
TFoo & { bar: string }
can be assigned toTFoo
, if there are any callbacks (ie. predicate) that passTFoo
back, typescript will attempts to assignTFoo
toTFoo & { bar: string }
, and the default of unknown has the same problem, this is a limitation of subtyping in typescript. To get around this we make the QueryFilter type a generic parameter which can adopt the type of the method argumentThis is impacting the new tRPC client which the QueryFilter typings were originally added for, in two ways:
Firstly the error type needs declaring because tRPC doesn't expect an Error to serialise across the wire, second once declaring this the data type shows up as incompatible because the QueryFilters type is ignored. This PR adds generic type parameters for QueryFilters to improve both situations