-
Notifications
You must be signed in to change notification settings - Fork 124
Modify type of query function's meta property to be more precise #857
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
Conversation
🦋 Changeset detectedLatest commit: af196fa The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
More templates
@tanstack/angular-db
@tanstack/db
@tanstack/db-ivm
@tanstack/electric-db-collection
@tanstack/offline-transactions
@tanstack/powersync-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/rxdb-db-collection
@tanstack/solid-db
@tanstack/svelte-db
@tanstack/trailbase-db-collection
@tanstack/vue-db
commit: |
|
Size Change: 0 B Total Size: 85.8 kB ℹ️ View Unchanged
|
|
Size Change: 0 B Total Size: 3.34 kB ℹ️ View Unchanged
|
samwillis
left a comment
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.
@kevin-dp this may be the recommended approach, but like you I don't really like it.
Could we just modify the type here:
db/packages/query-db-collection/src/query.ts
Lines 72 to 76 in 1527028
| queryFn: TQueryFn extends ( | |
| context: QueryFunctionContext<TQueryKey> | |
| ) => Promise<Array<any>> | |
| ? (context: QueryFunctionContext<TQueryKey>) => Promise<Array<T>> | |
| : TQueryFn |
such that it includes our metadata?
@samwillis i had given this a try before but i abandoned it early, so i tried again now with a bit more perseverance. I got close but i don't think this is the right approach: d895a98 Essentially, it works on our side but it breaks types when interacting with Tanstack Query features. For example: const queryFn = (
context: QueryCollectionFunctionContext // <-- this is our new context type
) => {
// ...
}
await queryClient.prefetchQuery({ queryKey, queryFn }) // expects queryFn to take a QueryFunctionContextThe call to If what i'm saying is not completely clear you can checkout the commit i linked and run the tests in the query package. You will see the error i'm describing above. |
samwillis
left a comment
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.
Great work, and thanks for digging into this issue. Let's go for this module interface.
Just needs a changeset.
f525e45 to
af196fa
Compare
Fixes #802
By default a Tanstack Query function's
metaproperty is typed asRecord<string, unknown> | undefined. However, the Query Collection actually extends the meta property with aloadSubsetOptionsproperty but this is not reflected in the type ofmeta.This PR augments the
@tanstack/query-coremodule such that themetaproperty is typed asRecord<string, unknown> & { loadSubsetOptions: LoadSubsetOptions }. I find it a bit odd that we have to resort to module augmentation but this seems to be the recommended approach.I added a type test that reproduces the problem.
Note: the type of the meta property still allows it to be
undefinedeven though we know that won't be the case. I didn't find an elegant way to solve this.