fix(client-ofetch): set ignoreResponseError default to false and handle FetchError response#3856
Conversation
…le FetchError response
|
|
|
@awdr74100 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:S
|
TL;DR — Fixes Key changes
Summary ｜ 30 files ｜ 3 commits ｜ base:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The runtime fix in bundle/client.ts and bundle/utils.ts is correct — I verified every load-bearing claim about ofetch against upstream source (unjs/ofetch): .raw() does throw FetchError on non-2xx when ignoreResponseError: false, FetchError.response is the same FetchResponse<T> with ._data populated before the error is constructed, and network failures yield a FetchError with .response === undefined which the new catch correctly rethrows.
However this PR is a breaking behavioral change for existing client-ofetch users (4xx/5xx responses now throw inside ofetch and flow through a different path), and it's missing the artifacts that ship with that kind of change in this repo:
- All generated snapshots are stale — all 11 dirs under
packages/openapi-ts-tests/main/test/__snapshots__/3.1.x/clients/@hey-api/client-ofetch/**/client/utils.gen.tsstill contain?? trueon line 304 andignoreResponseError: trueon line 504, plus none of them contain the newtry { $ofetch.raw(...) } catch { ... }block.examples/openapi-ts-ofetch/src/client/client/utils.gen.tsis also stale (same two lines). Snapshot tests will fail on CI until these are regenerated (pnpm tu -- @hey-api/openapi-ts+pnpm examples:generate). - No changeset —
.changeset/contains onlyREADME.md. Perdocs/openapi-ts/community/contributing/developing.mdand the precedent of #3814, semantic client-behavior flips require a changeset marked**BREAKING**:. - No
docs/openapi-ts/migrating.mdentry — thev0.97.0block documents analogous client breaking changes; this PR needs a peer entry under the next version. docs/openapi-ts/clients/ofetch.mdnever documentsignoreResponseErroror the new error-flow; at minimum, users need to know how to restore the pre-PR behavior (ignoreResponseError: true).
Inline comments cover a test-coverage gap for the central change. Nothing blocks from a correctness standpoint — the control flow is sound and the FetchError extraction path is faithful to ofetch's contract.
| ...jsonBodySerializer, | ||
| headers: defaultHeaders, | ||
| ignoreResponseError: true, | ||
| ignoreResponseError: false, |
There was a problem hiding this comment.
The central behavioral change of this PR is the flip from true to false here and at line 302, but no added test observes either value. A regression reverting one (or a typo letting the two defaults diverge) would pass the new suite because every new test uses mockRejectedValue(...) and never inspects the ofetch options. Consider adding expect(createClient({}).getConfig().ignoreResponseError).toBe(false) and/or a test that captures mockOfetch.raw.mock.calls[0][1].ignoreResponseError — the existing serialized request body handling suite already uses that pattern.
|
|
||
| expect(result.error).toEqual({ message: 'Not Found' }); | ||
| expect(result.response?.status).toBe(404); | ||
| }); |
There was a problem hiding this comment.
There's no test asserting the opt-out path (ignoreResponseError: true) still works as a backward-compat escape hatch. A future change forcing the option to false regardless of user input would go unnoticed. Consider a fourth test that passes ignoreResponseError: true, uses mockResolvedValue(response404) (not rejected), and asserts the client returns { error, response } without entering the new catch branch.
| try { | ||
| response = await $ofetch.raw(finalUrl, responseOptions); | ||
| } catch (fetchError: unknown) { | ||
| // When ignoreResponseError is false (default), ofetch throws FetchError for non-2xx. | ||
| // Extract the response so response interceptors can still process it before we | ||
| // decide whether to throw or return the error. | ||
| const maybeFetchError = fetchError as { response?: typeof response }; | ||
| if (maybeFetchError?.response) { | ||
| response = maybeFetchError.response; | ||
| } else { | ||
| throw fetchError; | ||
| } | ||
| } |
There was a problem hiding this comment.
Not blocking, but worth noting: real ofetch FetchError also exposes .status, .statusText, and .data as getters on the error itself (see unjs/ofetch src/error.ts). The mock in the new tests only mirrors .response, which is fine for what this block reads today but would invisibly drift from reality if fetchError.status-based branching (e.g. retry on specific codes) is added later. Not a change request — a note for whoever extends this catch.
dosubot
Bot
added
size:M
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3856 +/- ##
==========================================
+ Coverage 39.58% 39.68% +0.10%
==========================================
Files 532 532
Lines 19581 19587 +6
Branches 5835 5831 -4
==========================================
+ Hits 7751 7773 +22
+ Misses 9582 9572 -10
+ Partials 2248 2242 -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:
|
@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: |
1 participant
Problem
onResponseError was never called for 4xx/5xx responses because ignoreResponseError: true was
hardcoded as the default, preventing ofetch from triggering its error hook. This contradicts the
documented usage.
Even setting ignoreResponseError: false manually did not help — client.ts had no handling for the
FetchError thrown by ofetch, so result.response would be undefined and result.error would be the raw
FetchError object instead of the parsed response body.
Fix
before continuing through the response interceptor pipeline
This ensures:
Test plan