feat(vue-query): useQuery support ref args #5027
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 1062400:
|
|
Hi @DamianOsipiuk! Please check this PR, Welcome to discussion. Thank you. |
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 1062400. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this branch ✅ Successfully ran 2 targetsSent with 💌 from NxCloud. |
| | QueryFunction<TQueryFnData, UnwrapRef<TQueryKey>> | ||
| | UseQueryOptionsGeneric<TQueryFnData, TError, TData, TQueryKey> = {}, | ||
| arg3: UseQueryOptionsGeneric<TQueryFnData, TError, TData, TQueryKey> = {}, | ||
| | MaybeRef<QueryFunction<TQueryFnData, UnwrapRef<TQueryKey>>> |
There was a problem hiding this comment.
I'm not sure if setting the type of queryFn as MaybeRef is a wise choice. One reason is that it might cause more conflicts when users upgrade to v5.
Do you have any thoughts on this?
There was a problem hiding this comment.
I agree that is not wise choice too, but currently mutationFn type is already as MaybeRef
There was a problem hiding this comment.
I check parseQueryArgs again, I've realized that it doesn't support queryFn as a MaybeRef. I'll have to remove it in this case.
There was a problem hiding this comment.
query/packages/vue-query/src/useBaseQuery.ts
Lines 188 to 190 in 857ecd9
Currently, it should be possible to support queryFn as a ref. Regarding the transition to v5, I'm not sure if I'm overthinking it.
There was a problem hiding this comment.
Yes you're right !
However, I've also noticed another thing, it ensures that queryFn is a function in here:
query/packages/vue-query/src/types.ts
Lines 43 to 50 in 857ecd9
Since it's being handled here, should follow suit as well?
There was a problem hiding this comment.
I just noticed that the task aren't passing. I found that in src/__tests__/useQuery.test.ts at line 262, it's passing a ref to queryFn. It looks like we should reintroduce MaybeRef in this case.
There was a problem hiding this comment.
If that's the case, the types here might not be quite accurate, right? 🤯
query/packages/vue-query/src/types.ts
Lines 43 to 50 in 857ecd9
It would be nice to have feedback from @DamianOsipiuk to know which is expect.
There was a problem hiding this comment.
Although in theory queryFn is an option like any other, so it should be properly handled when it's changed to another one, I would strongly discourage it.
So if types currently do not allow it, let's leave it this way.
There was a problem hiding this comment.
@DamianOsipiuk Thank you for comment, we don't change any queryFn type currently.
There was a problem hiding this comment.
@DamianOsipiuk Please merge this PR, this is important for use queryOptions as component props ex:
const Comp = defineComponent({
props: {
queryOptions: Object,
},
setup: (props) => {
return useQuery(
computed(() => ({
...props.queryOptions,
queryFn: () => {
// ...
}
}))
)
}
})
Codecov ReportPatch coverage is ❗ Your organization needs to install the Codecov GitHub app to enable full functionality.
📢 Thoughts on this report? Let us know!. |
useMutation is support ref args already, but useQuery is not. so I set useQuery every arguments with MaybeRef just like useMutation