FEATURE-3879: added support for typescript isolatedDeclarations#3880
FEATURE-3879: added support for typescript isolatedDeclarations#3880tompuric wants to merge 7 commits into
Conversation
|
|
|
@tpuric is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
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.
Important
The SDK plugin's new .returns(...) clause is appended unconditionally — for the Nuxt client this emits RequestResult<Responses, Errors, ThrowOnError>, but client-nuxt's RequestResult is parameterized as <TComposable, ResT, TError>. Regenerating any Nuxt SDK after this change will produce an annotation that doesn't match the runtime call's generics. Worth gating the new .returns(...) behind !isNuxtClient (or threading a Nuxt-specific shape through it) before merge.
TL;DR — Adds explicit return-type and field-type annotations across the core bundle templates and the SDK v1 emitter so generated output satisfies isolatedDeclarations, and enables declaration + isolatedDeclarations in several example tsconfig.json files to lock the contract in.
Key changes
- Annotate client bundle helpers —
createQuerySerializer,setAuthParams,getUrl, path/object/primitive serializers,getValidRequestBody, andqueryKeyJsonReplacerget explicit return types acrossclient-angular,client-axios,client-core,client-fetch,client-ky,client-next,client-nuxt, andclient-ofetchbundle templates. - Type the generated
clientconst and SDK operations —client-core/client.tsnow imports aClienttype symbol and annotatesexport const client: Client = createClient(...); the SDK v1 emitter (sdk/v1/node.ts,sdk/v1/plugin.ts) registersRequestResultas an external symbol and appends a.returns(client.RequestResult<Responses, Errors, ThrowOnError, [responseStyle?]>)clause to every operation function. - Type a registry static field —
enrichRootClassadds an explicit.type($.type(symbolRegistry).generic(symbol))to the staticreadonlyfield so the class declaration is portable without inference. - Enable
isolatedDeclarationsin examples —tsconfig.jsoninaxios,fetch,ky,nestjs,next,openai(and others) adds"declaration": true, "isolatedDeclarations": true. - Annotate example app code — example
App.tsx,layout.tsx,page.tsx,tailwind.config.ts,pets.controller.ts,store.controller.ts, andopenapi-ts.config.tsfiles get explicit return types (React.ReactNode,Promise<UserConfig>, etc.).
Summary ｜ 150 files ｜ 1 commit ｜ base: main ← support-typescript-isolated-declarations-3879
Nuxt SDK return type uses the wrong generic shape
Before:
implementFnproduced operations with inferred return types, which TS was free to resolve through the Nuxt-specificRequestResult<TComposable, ResT, TError>.
After:.returns(client.RequestResult)is appended outside the$if(isNuxtClient, ...)branch with<Responses, Errors, ThrowOnError>generics — the positional argument shape that matches the non-Nuxt clients but not Nuxt.
packages/openapi-ts/src/plugins/@hey-api/client-nuxt/bundle/types.ts:92 declares RequestResult<TComposable extends Composable = '$fetch', ResT = unknown, TError = unknown>. The new emitter will pass an SDK's *Responses type as TComposable, which is structurally meaningless and will fail to extend Composable. The fix is to skip .returns(...) for Nuxt or to emit a different shape there — the existing $if(isNuxtClient, ...) block at the top of implementFn is the natural place.
sdk/v1/node.ts · client-nuxt/bundle/types.ts
Unrelated tsconfig.json flips in openapi-ts-next
Before:
"allowJs": truein the Next example.
After:"allowJs": falsealongside the newdeclaration/isolatedDeclarationsflags.
allowJs has nothing to do with isolated declarations; flipping it scopes a behavior change that isn't motivated by the PR title. The example's postcss.config.mjs is the only .mjs and likely isn't in the program, so it probably still builds, but the flip is worth either reverting or calling out explicitly in the PR description.
examples/openapi-ts-next/tsconfig.json
Mechanical generated-file churn dominates the diff
Roughly 130 of the 150 files are regenerated .gen.ts outputs across 16 example projects — they reflect the source-side annotations and look internally consistent. The substantive review surface is the ~15 source files under packages/openapi-ts/src/plugins/@hey-api/. Worth running pnpm test to confirm the snapshot test suites under packages/openapi-ts-tests/main/test/__snapshots__/3.1.x/clients/@hey-api/client-nuxt/** and other client snapshots are also regenerated — none of them appear in this diff, and they will diverge once this code lands.
client-core/client.ts · client-core/bundle/utils.ts · sdk/v1/plugin.ts
Claude Opus ｜ 𝕏
| .returns( | ||
| $.type(plugin.external('client.RequestResult')) | ||
| .generic( | ||
| plugin.querySymbol({ | ||
| category: 'type', | ||
| resource: 'operation', | ||
| resourceId: operation.id, | ||
| role: 'responses', | ||
| }) ?? 'unknown', | ||
| ) | ||
| .generic( | ||
| plugin.querySymbol({ | ||
| category: 'type', | ||
| resource: 'operation', | ||
| resourceId: operation.id, | ||
| role: 'errors', | ||
| }) ?? 'unknown', | ||
| ) | ||
| .generic('ThrowOnError') | ||
| .$if(plugin.config.responseStyle === 'data', (t) => | ||
| t.generic($.type.literal(plugin.config.responseStyle)), | ||
| ), |
There was a problem hiding this comment.
This .returns(...) is applied unconditionally, but the $if(isNuxtClient, ...) block above already adds Nuxt-specific generics (TComposable, TDefault) that don't fit RequestResult<Responses, Errors, ThrowOnError>. Nuxt's own RequestResult is <TComposable, ResT, TError> (see client-nuxt/bundle/types.ts:92), so the emitted annotation will pass a *Responses type where TComposable extends Composable is expected. Gate this clause behind !isNuxtClient (or emit the Nuxt shape there).
There was a problem hiding this comment.
I'm mostly concerned about fetch as that's our client so i'm not sure how Nuxt plays into this. The generated examples are essentially a demo of the potential output
There was a problem hiding this comment.
The SDK emitter is shared across all clients though — implementFn runs for every client including client-nuxt, so the unconditional .returns(client.RequestResult<Responses, Errors, ThrowOnError>) will be emitted into Nuxt-generated SDKs and won't match RequestResult<TComposable, ResT, TError> from client-nuxt/bundle/types.ts:92. The existing $if(isNuxtClient, ...) block at line 287 is the natural seam: either skip .returns(...) when isNuxtClient, or emit <TComposable, Responses, Errors> in that branch. Even if fetch is the priority, leaving Nuxt's emitted output broken under isolatedDeclarations is the kind of regression that surfaces later as a snapshot/build break.
Claude Opus ｜ 𝕏
| "target": "ES2017", | ||
| "lib": ["dom", "dom.iterable", "esnext"], | ||
| "allowJs": true, | ||
| "allowJs": false, |
There was a problem hiding this comment.
Flipping allowJs from true to false is unrelated to isolated declarations and should be either reverted or called out in the PR description. The example's postcss.config.mjs is the only loose script and isn't in the program, but the change still reads as scope creep.
There was a problem hiding this comment.
This was switched because openapi-ts build failure occurred with the reason being that allowJs: true being incompatible with isolatedDeclarations
There was a problem hiding this comment.
Makes sense — allowJs + isolatedDeclarations is incompatible without --declaration constraints on the JS surface, so flipping it here is justified. Worth a one-line note in the PR description so reviewers don't read it as scope creep.
Claude Opus ｜ 𝕏
| bodySerializer?: BodySerializer | null; | ||
| serializedBody?: unknown; | ||
| }) { | ||
| }): unknown { |
There was a problem hiding this comment.
getValidRequestBody's annotated return type unknown is correct (all branches return unknown | null | undefined) but it does erase the input narrowing callers previously got via inference. If any caller relies on a narrower inferred type, they'll need their own annotation — worth a quick grep before merging.
There was a problem hiding this comment.
Yep not ideal, feel free to provide suggestions
There was a problem hiding this comment.
Concretely: the function already returns unknown | null | undefined, so the explicit unknown is correct and no caller can narrow further from the signature alone — unknown widens what inference gave you, but only callers that did if (body) { ... } style refinement keep their narrowing through control flow. A grep for getValidRequestBody( in the client bundles shows every call site immediately passes the result to fetch/axios/etc., which accept unknown-ish bodies, so no follow-up annotation is required. If you wanted to preserve precision, a generic <T>(options: { body?: T; ... }): T | null | undefined would work, but it's bloat for zero practical benefit here.
Claude Opus ｜ 𝕏
|
@pullfrog reply to the comments |
|
Replied to all three author responses on the prior review: acknowledged the Task list (4/4 completed)
|
|
@tompuric will you update the snapshots? |
|
@tompuric should need just |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3880 +/- ##
==========================================
- Coverage 39.58% 39.50% -0.08%
==========================================
Files 532 533 +1
Lines 19581 19656 +75
Branches 5835 5852 +17
==========================================
+ Hits 7751 7766 +15
- Misses 9582 9639 +57
- Partials 2248 2251 +3
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:
|
|
@tompuric now it gives you actual type errors. Does that not happen on your end when you said the current implementation works for you? |
|
When I say works for me, I mean when i yarn patch my changes into our repo it's 👍
Edit: i'll see what I can do to get the PR ready here, but might not get to it until next week |
@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: |
| @@ -307,6 +317,31 @@ function implementFn<T extends ReturnType<typeof $.func | typeof $.method>>(args | |||
| ), | |||
| ) | |||
| .params(...opParameters.parameters) | |||
| .$if(!isNuxtClient && !hasServerSentEvents, (m) => | |||
There was a problem hiding this comment.
This needs to be validated. Had trouble getting these two integrated.
Question: should these paths be covered?
There was a problem hiding this comment.
I've resolved this and have covered all areas in a cleaner/easier-to-read approach
|
Hey @mrlubos , the PR is now in a ready state 🥳 I'm not sure how to improve the code coverage here for these changes so let me know if that's required. Let me know how else I can assist with this, thanks 🙏 |
tompuric
force-pushed
the
support-typescript-isolated-declarations-3879
branch
from
b63675a to
bf935da
Compare
3 participants
No description provided.