fix(#2204/#3150/#3519)!: proper error handling for all clients#3814
fix(#2204/#3150/#3519)!: proper error handling for all clients#3814SukkaW wants to merge 11 commits intohey-api:mainfrom
Conversation
|
|
|
@SukkaW is attempting to deploy a commit to the Hey API Team on Vercel. A member of the Team first needs to authorize it. |
|
|
TL;DR — Wraps the entire Key changes
Summary | 702 files | 11 commits | base: Unified
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3814 +/- ##
==========================================
- Coverage 39.84% 39.82% -0.02%
==========================================
Files 530 530
Lines 19475 19494 +19
Branches 5802 5818 +16
==========================================
+ Hits 7759 7764 +5
- Misses 9485 9493 +8
- Partials 2231 2237 +6
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:
|
There was a problem hiding this comment.
Important
Two correctness issues: double error-interceptor invocation in the ky client, and a type/runtime mismatch for response in the fetch client's error branch. The overall approach is sound — wrapping the full lifecycle in try/catch is the right fix for #3150 and #2204.
Good structural change overall. The pattern of extracting throwOnError/responseStyle before the try and declaring request/response as | undefined is clean and consistent across clients.
Not in diff — worth noting:
client-axiosstill hasbeforeRequest()outside its try block (line 72 ofclient-axios/bundle/client.ts), so it remains vulnerable to the exact issue this PR fixes for other clients. If this is intentional (e.g., deferred to a follow-up), it should be documented — the PR title says "all clients."client-nuxthas no try/catch at all, which may be intentional given Nuxt composables' error-handling model, but should be confirmed.
Task list (5/5 completed)
- Read the PR diff to identify major areas of change
- Investigate high-risk areas: try-catch wrapping logic in client plugins
- Check snapshot/test changes for correctness
- Impact analysis: breaking change assessment
- Draft and submit review
response/request object may become undefined after the changes, but in our test case we can always assert they do exists.
|
The author's commit Task list (5/5 completed)
|
|
Verified SukkaW's fixes in Task list (5/5 completed)
|
|
Verified that Task list (3/3 completed)
|
|
Replied to SukkaW's feedback on the ky client double error-interceptor invocation thread. After re-reading the code, the original finding still holds: when Task list (3/3 completed)
|
|
Verified and confirmed the fix in Task list (4/4 completed)
|
@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: |
|
@mrlubos I believe the PR is ready for review now, and
|

Fixes #2204
Fixes #3150
Fixes #3519
By fixing those issues, the PR has to change the behaviors of how clients handle errors and how error interceptors are invoked, thus should be considered as breaking changes. The PR has tried to minimize any breaking changes.
What this PR does:
throwOnError: falsemay still throw an error (instead of returning a result object{ error, data }), typically when request validation (via schema likezod,valibot, etc.) fails (as seen in Errors from Zod validator does not trigger the error interceptor #3150), or failed to build a request object (as seen in ThrowOnError not working #2204). Now this has changed. Clients now truly respectthrowOnError: trueand will always "swallow" all errors and return a result object{ error, data }.throwOnError: true, this won't affect you. But if you usethrowOnError: false, you should be aware that previously thrown errors may now be returned as part of the result object instead, and you should double check how you handle errors.requestfield andresponsefield (if any) may now beundefined. This is because an error may happen during the building of the request or be caused by network issues, in which case there won't be a request or response object, so you should check for the existence ofrequestandresponsebefore using them. The TypeScript types have been updated to reflect this changerequestand/orresponseparameters asundefinedwhen invoked (as seen in Incorectly typed error interceptor of fetch-client #3519). Same reason as above when we can't obtain a request or response object.client.interceptors.error), this won't affect you. But if you do, you should check therequestandresponseparameters in your error interceptor functions forundefinedbefore using them. The TypeScript types have been updated to reflect this change.client-fetch,client-ky, andclient-nextdo not invoke error interceptors with previous interceptors' returned error objects (but the original error object, discarding previous intermediate transformations). This is also now fixed. The new behavior should now be corrected, but it is a behavior change nonetheless.client.interceptors.error), or if you do, but aren't using the above-mentioned clients, you are not affected. But if you do, you should double-check what error object you are expecting.@mrlubos As usual, I didn't include a changeset in this PR. Feel free to push one as you see fit for the actual CHANGELOG.