fix(client): SSE iterator's TReturn was the endpoint's HTTP error union#3919
fix(client): SSE iterator's TReturn was the endpoint's HTTP error union#3919kitlangton wants to merge 1 commit into
Conversation
|
|
|
@kitlangton 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: 58962a0 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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Solid bug analysis and a minimal fix. One design question worth answering before merge.
Reviewed changes — SseFn's second generic was being plumbed into ServerSentEventsResult's TReturn slot (the async-iterator return-value type) instead of an error channel. The diff corrects all six affected client bundles and regenerates the matching snapshots.
- Drop
TErrorfromPromise<ServerSentEventsResult<...>>— forclient-fetch/client-axios/client-ky/client-next/client-ofetch/client-angular, soTReturndefaults tovoid(matching whatcreateSseClientactually returns atpackages/openapi-ts/src/plugins/@hey-api/client-core/bundle/serverSentEvents.ts:72,92). - Preserve
TErrorarity as_TError— kept the unused generic slot (with aneslint-disable-next-line @typescript-eslint/no-unused-varscomment, consistent with 8 other call sites in the repo) so the SDK template atpackages/openapi-ts/src/plugins/@hey-api/sdk/shared/operation.ts:475-477keeps emitting<TResponse, TError, ThrowOnError>uniformly without codegen changes. - Regenerate 196
types.gen.tssnapshots — the same 3-line edit propagated mechanically. - Add
.changeset/sse-iterator-return-type.md— single@hey-api/openapi-tspatch entry. - Nuxt is correctly left alone — Nuxt's
SseFnreturnsPromise<ServerSentEventsResult<RequestResult<TComposable, ResT, TError>>>(Nuxt threadsTErrorthroughRequestResult, filling slot 1 /TData, notTReturn), so the same bug doesn't exist there.
ℹ️ _TError + eslint-disable vs. dropping the generic from SDK emission
The PR body already flags this and it's the only real decision the diff leaves open. The current shape keeps SseFn's generic arity at 3 with an underscore-prefixed unused slot purely so sdk/shared/operation.ts:471-477 can keep emitting <TResponse, TError, ThrowOnError> for every method. The alternative — making the SDK template skip symbolErrorType when hasServerSentEvents is true and dropping TError from SseFn entirely — is a slightly larger change but removes the dead generic + lint suppression and stops other consumers of SseFn from being able to pass an error type that silently does nothing.
Two reasonable paths; the maintainer's call which one to ship.
Technical details
# `_TError` + `eslint-disable` vs. dropping the generic from SDK emission
## Affected sites
- `packages/openapi-ts/src/plugins/@hey-api/client-{fetch,axios,ky,next,ofetch,angular}/bundle/types.ts` — `SseFn` keeps a `_TError` slot purely for call-site arity compatibility.
- `packages/openapi-ts/src/plugins/@hey-api/sdk/shared/operation.ts:471-477` — uniformly emits `.generic(symbolErrorType ?? 'unknown').generic('ThrowOnError')` for every method, including SSE.
## Required outcome
- Pick one shape and stick with it across all six clients.
## Suggested approach
Two options, pick one:
1. **Ship as-is.** Smaller blast radius. The unused `_TError` slot is harmless and the underscore-prefix convention is idiomatic. Cost: any future consumer typing `client.sse.post<Foo, MyError>(…)` gets no compile-time signal that `MyError` is ignored.
2. **Drop `TError` from `SseFn` (3 → 2 generics) and update the SDK template.** In `sdk/shared/operation.ts:461-483`, branch the `.generic(...)` chain on `hasServerSentEvents` so SSE calls emit `<TResponse, ThrowOnError>` (no error slot). Removes the dead generic from the public surface entirely. Cost: touches codegen + regenerates the same 196 snapshots a second time.
## Open questions for the human
- Which direction do you want? Path 1 is what's currently in the PR. Path 2 is cleaner long-term but needs sign-off because it changes the emitted SDK call-site shape.ℹ️ Changeset entry is far longer than the repo convention
Existing changesets in .changeset/ are one-line **scope**: short description entries (e.g. cuddly-papayas-laugh.md, late-shirts-repeat.md). The new file packs the entire PR description into one paragraph. Not blocking — but worth trimming to a single line and letting the PR body / commit message carry the detail, for consistency with the surrounding changesets.
Claude Opus ｜ 𝕏
| "@hey-api/openapi-ts": patch | ||
| --- | ||
|
|
||
| **client**: fix `SseFn` returning an `AsyncGenerator` whose `TReturn` is the endpoint's HTTP error union. The error generic was being threaded into `ServerSentEventsResult`'s second generic slot, which is the iterator's return-value type — not an error channel. Harmless while error responses are `unknown`, but breaks every `.return()` call and every mock async generator the moment an SSE endpoint declares a concrete error type. Fix in all six affected client bundles (fetch/axios/ky/next/ofetch/angular) by dropping `TError` from `Promise<ServerSentEventsResult<TData, TError>>` so `TReturn` defaults to `void`, matching what `createSseClient` actually returns. The `TError` generic parameter is preserved (with an `eslint-disable` for the now-unused declaration) so call-site arity in the SDK template, which emits `<TResponse, TError, ThrowOnError>` uniformly for every method, keeps working without codegen changes. The Nuxt client's `SseFn` uses a different shape and was unaffected. |
There was a problem hiding this comment.
Changeset entries in this repo are one-line **scope**: short description — see cuddly-papayas-laugh.md / late-shirts-repeat.md. Worth trimming to e.g. **client**: fix SSE iterator's TReturn being the endpoint's HTTP error union and letting the PR body / commit message carry the detail.
| >( | ||
| options: Omit<RequestOptions<never, ThrowOnError>, 'method'>, | ||
| ) => Promise<ServerSentEventsResult<TData, TError>>; | ||
| ) => Promise<ServerSentEventsResult<TData>>; |
There was a problem hiding this comment.
Verified the analysis: ServerSentEventsResult<TData, TReturn = void, TNext = unknown> at client-core/bundle/serverSentEvents.ts:72, and createSseClient is signed as ServerSentEventsResult<TData> at :92 with no explicit return — so TReturn really is void and threading TError into slot 2 was wrong. The fix here is correct.
| >( | ||
| options: Omit<RequestOptions<never, ThrowOnError>, 'method'>, | ||
| ) => Promise<ServerSentEventsResult<TData, TError>>; | ||
| ) => Promise<ServerSentEventsResult<TData>>; |
There was a problem hiding this comment.
Same root cause as the other five clients — fix is correct and the _TError slot keeps the SDK template at sdk/shared/operation.ts:475-477 working without codegen changes.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3919 +/- ##
=======================================
Coverage 37.70% 37.70%
=======================================
Files 582 582
Lines 20844 20844
Branches 6063 6070 +7
=======================================
Hits 7860 7860
Misses 10570 10570
Partials 2414 2414
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:
|
|
@kitlangton is this related to #3018 in any way? That's another long-standing issue, wonder if both can be fixed at once here |
SseFn in every generated client (fetch / axios / ky / next / ofetch / angular) returns Promise<ServerSentEventsResult<TData, TError>>, but ServerSentEventsResult<TData, TReturn = void, TNext = unknown> — that second slot is the underlying AsyncGenerator's return-value type, not an error channel. HTTP errors are thrown from the initial promise or surfaced via onSseError; an async generator's .return() value has nothing to do with them. createSseClient itself signs as ServerSentEventsResult<TData> (no TError), and the inner async function* has no explicit return, so the real TReturn is void. Only the public wrapper type is wrong. Latent for ~9 months since d43ef3f (feat(client): add support for server-sent events). Harmless while users' SSE error responses were effectively unknown, but breaks every .return() call and every mock async generator the moment any SSE endpoint declares a concrete error response — they suddenly fail to satisfy an iterator-return contract that should never have existed. Drop TError from the Promise<ServerSentEventsResult<...>> return type so TReturn defaults to void. The TError generic parameter is preserved (renamed to _TError so noUnusedParameters lets it through, with an eslint-disable for the corresponding lint rule) so the SDK template, which emits <TResponse, TError, ThrowOnError> uniformly for every method, keeps working without codegen changes. Nuxt's SseFn uses a different shape and was unaffected.
kitlangton
force-pushed
the
fix/sse-iterator-treturn-miswiring
branch
from
9093357 to
58962a0
Compare
@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: |
|
They're in the same area but they fix different things. #3018 is a runtime change. It updates what the iterator yields on each tick, so that This PR is type-only and it changes a different slot. The two PRs touch different files and different lines, so there is no overlap and they can merge in either order. I am happy to rebase on whichever one lands first. |
2 participants
Problem
SseFnin every generated client (fetch,axios,ky,next,ofetch,angular) returnsPromise<ServerSentEventsResult<TData, TError>>. But:That second slot is the underlying
AsyncGenerator's return-value type — not an error channel. HTTP errors are thrown from the initial promise or surfaced viaonSseError; an async generator's.return()value has nothing to do with them.The SSE implementation itself agrees:
createSseClientsigns asServerSentEventsResult<TData>(noTError), and its innerasync function*has no explicit return statement, so the realTReturnisvoid. Only the publicSseFnwrapper type passes the wrong generic.This has been latent since
d43ef3f3b("feat(client): add support for server-sent events", Aug 2025). It's harmless while consumers' SSE error responses are effectivelyunknown— but it breaks every.return()call and every mock async generator the moment any SSE endpoint declares a concrete error type.Repro
We hit this in the wild after a downstream codegen added concrete error responses to an SSE endpoint — see opencode/pull/28503 for the consumer-side patch we shipped to unblock CI while this PR is in flight.
Fix
Drop
TErrorfrom thePromise<ServerSentEventsResult<...>>return type in all six affected client bundles soTReturndefaults tovoid. TheTErrorgeneric parameter is preserved (renamed to_TErrorsonoUnusedParameterslets it through, plus aneslint-disablefor the lint rule) so the SDK template — which emits<TResponse, TError, ThrowOnError>uniformly for every method — keeps working without codegen changes.Nuxt's
SseFnuses a different shape (itsTErrorflows intoRequestResult, not intoServerSentEventsResult'sTReturn) and was unaffected.The 196 snapshot churn is mechanical — same edit as the source files. Regenerating instead of hand-editing produces identical output (verified locally).
Test plan
pnpm typecheck— 37/37 packagespnpm vitest runacross@test/openapi-ts,@test/openapi-ts-zod-v4,@test/openapi-ts-valibot-v1,@test/openapi-ts-tanstack-query-v5,@test/openapi-ts-sdks— 717/717 tests passpnpm lint:fix— cleanFiled as draft to invite discussion on the
_TError+eslint-disableshape — happy to instead update the SDK generator template (packages/openapi-ts/src/plugins/@hey-api/sdk/shared/operation.ts:458-481) to stop emittingsymbolErrorTypefor SSE calls, which would let us drop theTErrorgeneric fromSseFnentirely. That's a slightly larger but cleaner change; let me know which direction you'd prefer.