feat: make TanStack Query plugin respect responseStyle: 'fields'#3662
feat: make TanStack Query plugin respect responseStyle: 'fields'#3662JorrinKievit wants to merge 4 commits intohey-api:mainfrom
Conversation
|
|
|
Someone is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
|
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3662 +/- ##
==========================================
- Coverage 38.99% 38.83% -0.17%
==========================================
Files 515 515
Lines 18901 19005 +104
Branches 5591 5605 +14
==========================================
+ Hits 7370 7380 +10
- Misses 9326 9410 +84
- Partials 2205 2215 +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: |
|
Reviewed PR #3662. Found a critical runtime/type mismatch bug where the default Task list (5/5 completed)
 |
dosubot
bot
added
size:L
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for working on this — the feature is genuinely useful and the overall design (plugin-level config + per-query TStyle override) is solid. There is one critical runtime bug and a few other issues that need addressing before this can merge.
Critical: The runtime conditional options?.responseStyle === 'fields' does not match when responseStyle is undefined (the common case when the user omits it), yet the type-level default for TStyle is 'fields'. This causes a type-safety violation where TypeScript thinks the return is { data, request, response } but the actual runtime value is just the unwrapped data.
Other issues: a regression where .export(plugin.config.mutationOptions.exported) was changed to .export(), duplicate imports, and heavy code duplication across three files that could be reduced significantly.
| .prop('responseStyle', $.literal('fields')), | ||
| ) | ||
| .await(), | ||
| ); |
There was a problem hiding this comment.
Bug (runtime/type mismatch): When the user doesn't pass responseStyle at all (the common case), options?.responseStyle is undefined. The check === 'fields' evaluates to false, so the code returns result.data. But the type-level default is TStyle = 'fields', meaning TypeScript believes the return type is { data, request, response } when it's actually just the raw data.
The condition should be inverted to check for the opt-out case:
const _data = options?.responseStyle === 'data' ? result.data : result;This same bug appears in mutationOptions.ts and infiniteQueryOptions.ts.
| ); | ||
| plugin.node(statement); | ||
| const statement = $.const(symbolMutationOptions) | ||
| .export() |
There was a problem hiding this comment.
Regression: The original code used .export(plugin.config.mutationOptions.exported) to respect the user's export configuration. This was changed to .export() which unconditionally exports. The if branch on line 103 correctly passes plugin.config.mutationOptions.exported — this else branch should do the same.
| .export() | |
| .export(plugin.config.mutationOptions.exported) |
| import { TsDsl } from '../../../../ts-dsl'; | ||
| import { $ } from '../../../../ts-dsl'; |
There was a problem hiding this comment.
Duplicate import — TsDsl and $ can be imported from the same specifier in a single statement.
| import { TsDsl } from '../../../../ts-dsl'; | |
| import { $ } from '../../../../ts-dsl'; | |
| import { $, TsDsl } from '../../../../ts-dsl'; |
| ); | ||
| plugin.node(statement); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Structure: The if/else branches duplicate nearly the entire function body (~120 lines each). The only meaningful differences are:
- An extra
TStylegeneric parameter ResponseResult/ResponseErrorwrapper types vs raw types- The
responseStyle: 'fields'prop on the SDK call - The ternary + cast return instead of a direct return
Consider extracting the shared logic (symbol creation, awaitSdkFn construction, queryOptionsObj shape, statement assembly) and only branching on the parts that differ. This would make the code easier to maintain and less prone to the kind of divergence bugs already present (like the .export() regression in mutationOptions.ts).
| const awaitSdkFn = $.lazy((ctx) => | ||
| ctx | ||
| .access( | ||
| plugin.referenceSymbol({ |
There was a problem hiding this comment.
The options parameter for the outer function is typed as Options<Data> & { responseStyle?: TStyle }, but inside queryFn, options is captured in the closure. When TanStack Query calls queryFn, the options reference here is the outer function's parameter — this is correct. But note that the SDK call spreads ...options which also includes responseStyle from the user. Since responseStyle: 'fields' appears after the spread, it always wins — this is fine, just want to confirm this override-by-position is intentional rather than accidental.
| if (opts.throwOnError) { | ||
| if (opts.responseStyle === 'fields') { | ||
| throw { | ||
| error: finalError, |
There was a problem hiding this comment.
The client changes add responseStyle-aware error throwing in the throwOnError path across all four client bundles. These are generated/bundled files — do these need corresponding type updates to the client's RequestOptions or Config types so that opts.responseStyle doesn't produce a type error? If responseStyle is already part of the Config type then this is fine, but worth verifying the types line up.
1 participant
Problem
When using the TanStack Query plugin, there's no way to access the HTTP
Responseobject — status codes, headers, or request metadata. The plugin always destructures{ data }from the SDK response and discards everything else, even whenresponseStyle: 'fields'is configured.This makes it impossible to:
X-Total-Count, rate-limit headers)QueryCache.onErrorRelated issues: #3628, #3632, #2070, #1762
Solution
This PR makes the TanStack Query plugin actually respect
responseStyle. A newresponseStyleconfig option is added to the plugin. I decided to have "data" as the default, since that kinda is happening right now for the plugin.Plugin-level config
Per-query override
Each generated function accepts a generic
TStyleparameter, so you can override per-query without changing the global default:Also works the other way — if the plugin default is
'fields', you can override specific queries back to'data'.Discussion point
For me the primary reason is being able to access the response type on Errors so I can handle them globally. But also included it for succesfull responses because I saw other issues.
What I don't like is that I kinda want to enable it globally, but only for Errors and not for the data itself. Because that would mean I am gonna have nested data objects everywhere 🤔