fix: forward TResponse generic through SDK Options type for SSE endpo…#3466
fix: forward TResponse generic through SDK Options type for SSE endpo…#3466bilby91 wants to merge 1 commit intohey-api:mainfrom
Conversation
|
|
|
Leaping into action... |
|
|
@bilby91 is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3466 +/- ##
==========================================
+ Coverage 38.99% 39.25% +0.26%
==========================================
Files 513 513
Lines 18768 18781 +13
Branches 5565 5577 +12
==========================================
+ Hits 7319 7373 +54
+ Misses 9252 9219 -33
+ Partials 2197 2189 -8
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:
|
| if (throwOnError) { | ||
| return $.type(symbolOptions) | ||
| .generic(isDataAllowed ? (symbolDataType ?? 'unknown') : 'never') | ||
| .generic(throwOnError) | ||
| .generic(symbolResponseType ?? 'unknown'); | ||
| } | ||
| return $.type(symbolOptions) | ||
| .generic(isDataAllowed ? (symbolDataType ?? 'unknown') : 'never') | ||
| .generic('boolean') | ||
| .generic(symbolResponseType ?? 'unknown'); |
There was a problem hiding this comment.
@bilby91 can you use .$if here? It should be more readable then
EDIT: no need for .$if, .generic(throwOnError !== undefined ? throwOnError : 'boolean') should work
There was a problem hiding this comment.
This should be resolved.
| * Get events | ||
| */ | ||
| export const eventSubscribe = <ThrowOnError extends boolean = false>(options?: Options<EventSubscribeData, ThrowOnError>) => (options?.client ?? client).sse.get<EventSubscribeResponses, unknown, ThrowOnError>({ url: '/event', ...options }); | ||
| export const eventSubscribe = <ThrowOnError extends boolean = false>(options?: Options<EventSubscribeData, ThrowOnError, EventSubscribeResponse>) => (options?.client ?? client).sse.get<EventSubscribeResponse, unknown, ThrowOnError>({ url: '/event', ...options }); |
There was a problem hiding this comment.
@bilby91 is this client.sse.get<T> change expected? Seems it was already typed and this changes the type
There was a problem hiding this comment.
I went back and reviewed the code more carefully. I believe this change is intentional.
Before: client.sse.get<EventSubscribeResponses, ...> where EventSubscribeResponses = { 200: Event } — a status-code-keyed object. This flows as TData into StreamEvent, so onSseEvent gets event.data: { 200: Event } instead of event.data: Event.
After: client.sse.get<EventSubscribeResponse, ...> where EventSubscribeResponse = Event — the unwrapped success type. Now onSseEvent correctly gets event.data: Event.
From what I can tell, the stream async generator wasn't affected because ServerSentEventsResult already handles the unwrapping via TData extends Record<string, unknown> ? TData[keyof TData] : TData. But StreamEvent (used by onSseEvent) has no such unwrapping — it uses data: TData directly. So using Response instead of Responses seems needed to get the correct type for onSseEvent.
Let me know if I'm missing something!
There was a problem hiding this comment.
@bilby91 this change breaks typing for the stream. The current version works like this:
Notice onSseEvent() is not typed and the stream is typed. Compare to your pull request:
Now onSseEvent() is typed as you correctly claim, but the stream type is broken
There was a problem hiding this comment.
I see what you mean. Will get it fixed. We definitely don't want to break opencode, we also using it :)
|
@mrlubos Let me review this again. There is so much autogeneration here is kind of hard sometimes. This is the patch that I'm currently running in my project to fix the three bugs I reported yesterday. This patch on top of 0.92.3 is working fine for me. |
I can relate! |
e7c2994 to
f30daba
Compare
@hey-api/codegen-core
@hey-api/json-schema-ref-parser
@hey-api/nuxt
@hey-api/openapi-ts
@hey-api/shared
@hey-api/types
@hey-api/vite-plugin
commit: |
|
@mrlubos PR should be passing now. I finally had more time to look at this with Claude :) |
| * Get events | ||
| */ | ||
| export const eventSubscribe = <ThrowOnError extends boolean = false>(options?: Options<EventSubscribeData, ThrowOnError>) => (options?.client ?? client).sse.get<EventSubscribeResponses, unknown, ThrowOnError>({ url: '/event', ...options }); | ||
| export const eventSubscribe = <ThrowOnError extends boolean = false>(options?: Options<EventSubscribeData, ThrowOnError, EventSubscribeResponse>) => (options?.client ?? client).sse.get<EventSubscribeResponse, unknown, ThrowOnError>({ url: '/event', ...options }); |
There was a problem hiding this comment.
@bilby91 this change breaks typing for the stream. The current version works like this:
Notice onSseEvent() is not typed and the stream is typed. Compare to your pull request:
Now onSseEvent() is typed as you correctly claim, but the stream type is broken
f30daba to
3a260ef
Compare
…ints The onSseEvent callback was typed as StreamEvent<unknown> because the response type was never threaded through the generated Options type on the non-Nuxt path. This adds a TResponse generic to the SDK Options type alias and forwards it to the client Options, then passes the unwrapped response type for SSE endpoints so event.data is correctly typed. Closes hey-api#3463 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3a260ef to
32c0ce3
Compare

…ints
The onSseEvent callback was typed as StreamEvent because the response type was never threaded through the generated Options type on the non-Nuxt path. This adds a TResponse generic to the SDK Options type alias and forwards it to the client Options, then passes the unwrapped response type for SSE endpoints so event.data is correctly typed.
Closes #3463