-
-
Notifications
You must be signed in to change notification settings - Fork 172
Vue Query v5 #711
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
base: main
Are you sure you want to change the base?
Vue Query v5 #711
Conversation
🦋 Changeset detectedLatest commit: 8ba1a04 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
@fruchtzwerg is attempting to deploy a commit to the ts-rest Team on Vercel. A member of the Team first needs to authorize it. |
View your CI Pipeline Execution ↗ for commit 8ba1a04.
☁️ Nx Cloud last updated this comment at |
|
Hi @fruchtzwerg! Thanks so much for the PR, I didn't have a huge role in the implementation of the v5 react query adapter from @Gabrola but I've put reviewing this on my shortlist. I'll try get on it shortly. Appreciate your patience! |
Great work @fruchtzwerg - I ended up pulling down your branch and testing it against my (proprietary, sorry) code - the V5 types work great but I'm finding with the V4 types I'm getting The way that I integrated it is very weird (pretty much copy-pasting the code into my own fork) so it might have been a problem of my own creation, but thought I'd mention it in case someone else runs into the same problem and ends up searching for what caused it the same way I did. |
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.
I've added some comments based on my experience pulling this down and using it on my codebase, hope I'm not intruding :).
The other bit of feedback I have is that making it so that you have to get the query client out of a composable makes it quite difficult to use e.g. fetchQuery
in routes. An escape hatch where you can statically get a client as long as your client args stay static would be great.
fetchQuery<TData = TQueryFnData>( | ||
options: FetchQueryOptions<TQueryFnData, TError, TData> & | ||
TsRestQueryOptions<TAppRoute, TClientArgs>, | ||
): Promise<TData>; | ||
|
||
prefetchQuery<TData = TQueryFnData>( | ||
options: FetchQueryOptions<TQueryFnData, TError, TData, QueryKey> & | ||
TsRestQueryOptions<TAppRoute, TClientArgs>, | ||
): Promise<void>; | ||
|
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.
fetchQuery<TData = TQueryFnData>( | |
options: FetchQueryOptions<TQueryFnData, TError, TData> & | |
TsRestQueryOptions<TAppRoute, TClientArgs>, | |
): Promise<TData>; | |
prefetchQuery<TData = TQueryFnData>( | |
options: FetchQueryOptions<TQueryFnData, TError, TData, QueryKey> & | |
TsRestQueryOptions<TAppRoute, TClientArgs>, | |
): Promise<void>; | |
fetchQuery<TData = TQueryFnData>( | |
options: FetchQueryOptions<TQueryFnData, TError, TData, TQueryKey> & | |
TsRestQueryOptions<TAppRoute, TClientArgs, TQueryKey>, | |
): Promise<TData>; | |
prefetchQuery<TData = TQueryFnData>( | |
options: FetchQueryOptions<TQueryFnData, TError, TData, TQueryKey> & | |
TsRestQueryOptions<TAppRoute, TClientArgs, TQueryKey>, | |
): Promise<void>; | |
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.
if you have a non-unknown[]
typed query key this won't accept it for fetchQuery
currently, this change fixes it for me.
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.
Thank you @AlexGilleranGP for your feedback.
Apparently I had missed the contextual function types here as well.
Wouldn't have caught this otherwise. 👍
> { | ||
mutate: AppRouteFunction<TAppRoute, TClientArgs>; | ||
|
||
useMutation<TContext = unknown>( |
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.
I'm finding that even with the v5 types, on a route that's sufficiently nested (e.g. users.current.totp.setEnabled
), without removing this generic I fall victim to TS2589: Type instantiation is excessively deep and possibly infinite
when calling useMutation
.
I assume this is because routes on the contract end up being a deep recursive type (RecursivelyApplyOptions<RecursivelyApplyOptions<RecursivelyApplyOptions<
) for every level of nesting, then TsRestVueQueryHooksContainer
adds another n
layers of recursive type to it (no idea why removing <Context>
helps however, I assume that's just the straw that breaks the camel's back).
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.
-
I'd rather not fix this in the initial release of v5, beacause these types are copied straight from the react-query implementation. Therefore these issues will probably exist in react as well.
To allow the maintainers to copy future updates straight into vue-query, I tried to maintain as much consistency as possible. This will increase the chances that the vue-query implementation will not end up as abandoneware. -
Removing the generic for the context here would also remove typesafety for the
onMutate()
result:
client.createPost.useMutation({
onMutate: async (variables) => ({ title: variables.body.title }), // <-- define context
onSuccess: (_data, _variables, context) => {
console.log(context.title); // <-- context is of type { title: string }
},
});
If this is an issue for you and you cannot flatten your contract definition, I'd suggest opening a new issue after the release vue-query v5, so it can be addressed for both react and vue.
|
Could you provide an example of what you are trying to achieve? The Here is an example. The vanilla Tanstack VueQueryPlugin also relies on |
Thanks for taking a look @fruchtzwerg :) Right now I get a QueryClient with It is possible that I should be able to use the composable in my route handling and I just haven't got it configured correctly, but trying to invoke |
This PR brings Vue Query in line with React Query v4/v5 support.
v4 gets a small update to its types to address #703
v5 mostly copies
react-query-v5
with the necessary adjustments for VueNotable changes:
vue-query-v5/src/v5/create-ts-rest-plugin.ts
).MaybeRef<T>
type.I have not looked at Nuxt/SSR support, yet. This is about as fully featured as I can currently make it.
Tested in one of my own projects via local npm link.
Please let me know if something is amiss.