feat(@tanstack/query): add queryOptions override parameter to generated useQuery hooks#3528
feat(@tanstack/query): add queryOptions override parameter to generated useQuery hooks#3528nmokkenstorm wants to merge 2 commits into
Conversation
|
|
|
@nmokkenstorm is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: 7c1ecdb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
6d07fd5 to
cf8e859
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3528 +/- ##
==========================================
+ Coverage 39.22% 39.55% +0.33%
==========================================
Files 595 596 +1
Lines 21124 21139 +15
Branches 6151 6153 +2
==========================================
+ Hits 8285 8362 +77
+ Misses 10406 10354 -52
+ Partials 2433 2423 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@hey-api/codegen-core
@hey-api/json-schema-ref-parser
@hey-api/nuxt
@hey-api/openapi-ts
@hey-api/shared
@hey-api/spec-types
@hey-api/types
@hey-api/vite-plugin
commit: |
|
@nmokkenstorm Why is it an optional configuration flag? Why not add it for everyone? |
|
@nandorojo will like this |
|
@nmokkenstorm I updated your description with linked issue |
habit to make it opt-in tbh. I think the skipToken is a breaking change too on type level and it also slightly increases the generated bundle size but I will check. |
|
@nmokkenstorm I didn't look at the code yet, stopped after seeing the configuration haha. If it's going to be breaking either way, I'd much rather deprecate the old approach because we wouldn't want new users to start with the flag disabled, realize one day they need to enable it, and face a lot of breaking changes. We can talk through it when you feel the pull request is more ready. I'm pretty sure there was another thread where we talked about skip tokens but can't find it, I'd like to ideally avoid another breaking change when that thread gets addressed |
I'll double check, I think it's definately doable to have it be backwards compatible without a breaking change. The useQuery/Mutation stuff was opt in so I stuck to the pattern but I think this can be done without losing ergonomics. I'll update the pr description and ping you if it's reviewable, I needed a remote public ref to do some internal testing with first anyways. Thanks for the feedback! |
7df42fb to
3007b3c
Compare
|
@nmokkenstorm Can you clarify what do you mean by "add useQuery hooks" in the title? React/Preact Query already support generating useQuery hooks so I'm confused what it's referencing |
its about having the generated useQuery hooks exist per entity, with queryoptions and skiptoken enabled. the reason it's named like that is because the original code/branch orginates from a fork that pre-dates the support we had in our project. still tinkering on this, nothing to see/review here yet! |
|
Just making sure we're aligned on the scope! I shall retreat once more |
d6ebc7c to
b1fc4b9
Compare
b1fc4b9 to
2ffe100
Compare
d8bbba0 to
5eb6a92
Compare
|
@mrlubos I think it does what it needs to do now, below a redacted diff of how we're currently using it in the fork. I ran into an issue where the auto paginated version of useQuery has ts-ignore inserted and I needed it somewhere else to stay compliant, so I added support for a new case to the DSL for that. Probably worthwhile to check a) if that's correct and b) if maybe we can improve the related code to no longer need the ts-ignore, but that seemed a bit out of scope for this PR. lmk what you think? I'll see if I can get the merge checks to behave a bit more but I'm not sure what I actually can and can not influence there, you mentioned they are a bit flaky before. Before (query)import { getSomeResourceOptions } from '@org/api';
import { useQuery } from '@tanstack/react-query';
const { data, isLoading } = useQuery({
...getSomeResourceOptions({ path: { id: id ?? '' } }),
enabled: !!id,
});After (query)import { useGetSomeResource } from '@org/api';
import { skipToken } from '@tanstack/react-query';
const { data, isLoading } = useGetSomeResource(
id ? { path: { id } } : skipToken
);Before (query with extra options)import { getStatusOptions } from '@org/api';
import { useQuery } from '@tanstack/react-query';
const { data, isLoading } = useQuery({
...getStatusOptions(),
enabled: shouldPoll,
refetchInterval: shouldPoll ? 5000 : false,
});After (query with extra options)import { useGetStatus } from '@org/api';
const { data, isLoading } = useGetStatus({
queryOptions: {
enabled: shouldPoll,
refetchInterval: shouldPoll ? 5000 : false,
},
});Before (mutation)import { updateResourceMutation, getResourceQueryKey } from '@org/api';
import { useMutation, useQueryClient } from '@tanstack/react-query';
const queryClient = useQueryClient();
const { mutate, isPending } = useMutation({
...updateResourceMutation(),
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: getResourceQueryKey() });
},
});After (mutation)import { useUpdateResource, getResourceQueryKey } from '@org/api';
import { useQueryClient } from '@tanstack/react-query';
const queryClient = useQueryClient();
const { mutate, isPending } = useUpdateResource({
onSuccess: () => {
queryClient.invalidateQueries({ queryKey: getResourceQueryKey() });
},
}); |
|
Reviewed PR #3528. The Task list (5/5 completed)
|
2071133 to
b0ccb2b
Compare
|
@mrlubos all green except the vercel thing. I have some redacted real world usage before/after below, happy to chat if you have questions/concerns! Generated output:
|
|
@pullfrog review |
|
Reviewed PR #3528. The implementation is clean and correct — Task list (5/5 completed)
|
There was a problem hiding this comment.
Reviewed — no issues found.
Task list (5/5 completed)
- Read diff - source code changes (plugin logic, config, types)
- Read diff - test changes
- Read diff - snapshot changes (sampling)
- Trace data flow and verify correctness
- Self-critique and submit review
|
@mrlubos I see some conflicts have crept in, happy to resolve them; do you need anything else to get this reviewed / merged while I'm at it? |
51a80e4 to
a791fdd
Compare
|
All in favor of the skip token work thanks. One question: this auto-generates query hooks based on the query options and make that behavior the default. I'd actually prefer that the hooks NOT be generated by default. The query options do mostly everything we need and I think auto generated custom hooks are now abstraction dead weight in my opnion. To quote from https://tkdodo.eu/blog/the-query-options-api#query-factories
Perfectly fine having the option to generate hooks but it's extra code that I think people should ask for directy. |
No problem!
This was the original behaviour, there was some discussion about this upthread. I sort of I agree with what you're saying for different reasons, I think in an ideal scenario I either import the hook straight up from the generated file because I'm not doing something complicated. If I'm doing something complicated with the custom options (and probably/possibly reusing that throughout my app) I specific it separately by introducing a new file. I agree with you that creating a file just for re-exporting the generated hook doesn't make for the best ergonomics. @mrlubos and I had some talk here and elsewhere about BC and why the current approach makes sense, but I must say it has been a while and I reworked this code a bit over time so maybe if we talk again we might get to a different conclusion Either way thanks for your insight! Happy to work a bit more on this if needed to get it merged, once in a while I rebase with main and fix any conflicts/issues that surface, I'd be more than happy to receive some instruction from the maintainers to see what we can do to get this in. Fwiw the project I'm working on that's appreciating the missing/added functionality by this PR got a lot leaner for it. |
|
@nmokkenstorm is there a world where we could get the skipToken support without the hooks? Second everything else that was said above, my question is purely from the design perspective. |
I was a bit confused why the two where coupled together as well |
Hmmm they're not necessarily intrinsically linked, although it does make sense to implement them at least in a way that behaves well together. The way the skiptoken works is sort of as an override/overload to the query options, so any implementation for either should at least be aware of both. I can split it up in two separate (stacked?) PRs and explain the design decisions behind them if that makes reviewing easier. |
291119b to
6ce8096
Compare
6ce8096 to
2410551
Compare
80d2e32 to
e57f9a2
Compare
…ed useQuery hooks
e57f9a2 to
c2b4e46
Compare
@mrlubos @DogPawHat it's split now, slightly unsure on the branch hygiene, would love you input. this branch is now query options only, the linked branch is skiptoken, I have half a thought to retarget this PR to the other branch, but I don't know how well that works with your review flow. Alternative is to just merge the other branch first, keep this in draft and I do the integration of both features. Either way both need some additional testing on my end because the code diverges slightly from the original approach, but I think especially the skiptoken stuff should be fairly straightforward to test/review, lmk if you run into any issues! |
# Conflicts: # packages/openapi-ts/src/plugins/@tanstack/query-core/v5/useQuery.ts
There was a problem hiding this comment.
Reviewed changes — adds a queryOptions override parameter to generated useQuery hooks, mirroring the existing mutationOptions pattern on useMutation hooks. The change lets consumers pass TanStack Query options like staleTime or enabled per call site without losing type safety.
- Introduce
UseQueryParams<TFactory>type alias inpackages/openapi-ts/src/plugins/@tanstack/query-core/shared/useQueryParams.ts— a shared utility type that intersects the SDK options with an optionalqueryOptionsoverride typed asPartial<Omit<ReturnType<TFactory>, 'queryKey' | 'queryFn'>>. - Update
createUseQueryemitter inpackages/openapi-ts/src/plugins/@tanstack/query-core/v5/useQuery.ts— replaces the directOptions<TData>parameter withUseQueryParams<typeof optionsFn>, destructuresqueryOptionsfrom the parameter, and spreads it after the computed options so overrides take precedence. - Add unit test in
packages/openapi-ts/src/plugins/@tanstack/query-core/v5/__tests__/useQuery.test.ts— verifies the emitted AST contains the type alias and the correct hook body. - Add snapshot test scenario
useQuery-enabledacross OpenAPI 2.0.x, 3.0.x, and 3.1.x — covers generated output whenuseQuery: trueis set in plugin config. - Add changeset for a minor version bump of
@hey-api/openapi-ts.
Kimi K2 | 𝕏

Summary
When
useQueryhook generation is enabled, generated hooks now accept an optionalqueryOptionsproperty that is spread over the computed query options, which is the same as the existingmutationOptionsoverride onuseMutationhooks. This lets consumers override TanStack Query options without losing type safety.Independent of #3926 (skipToken support). Whichever PR merges first, the other will need a small
useQuery.tsrebase to combine both features.