fix(clients): defer URL construction and thread finalError through interceptors (#3803)#3851
fix(clients): defer URL construction and thread finalError through interceptors (#3803)#3851aartisonigra wants to merge 3 commits intohey-api:mainfrom
Conversation
|
|
|
@aartisonigra 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: 3740600 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 |
dosubot
Bot
added
the
size:L
|
TL;DR — Reorders the request lifecycle in Key changes
Summary ｜ 2 files ｜ 3 commits ｜ base: Reactive URL construction in
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
This PR should not be merged. It has two independent showstoppers, regresses behavior that is already correct on main, and does not fix the issue it claims to fix.
1. new Request('' as string, requestInit) throws a synchronous TypeError: Failed to parse URL from in Node.js (undici). An empty string is not a valid WHATWG Request URL, and Node has no default base URL unless setGlobalOrigin is called. Every request through custom-client, client-fetch, and client-ky fails immediately in SSR, serverless, CLI, and Vitest/Jest Node environments. Browsers resolve against document.baseURI and may hide this locally, but CI across Node 22/24 will fail outright.
2. The PR fixes the wrong clients. Issue #3803 is explicitly and exclusively about @hey-api/client-next, whose interceptor signature is (opts) => void and where moving buildUrl(resolvedOpts) to after the interceptor loop is a one-line fix. packages/openapi-ts/src/plugins/@hey-api/client-next/bundle/client.ts is not modified by this PR — the linked issue is still open after merge.
3. Error-interceptor threading already landed on main via #3814 for client-fetch, client-ky, and client-next (see CHANGELOG.md: "Breaking: pass previous result to error interceptors (#3814)" and docs/openapi-ts/migrating.md). The error-loop bodies in this PR are byte-identical to main; only comments were added. The PR description's "Error Threading" bullet is inaccurate for fetch and ky. custom-client is the only client where error threading is a real behavior change in this diff — and it has no changeset, no migration note, and no test.
4. For client-fetch and client-ky, interceptors take (request, options) => Request and mutate by returning a (possibly new) Request. The documented auth pattern at docs/openapi-ts/clients/fetch.md and docs/openapi-ts/clients/ky.md is request.headers.set('Authorization', ...); return request. This PR runs interceptors on a placeholder Request and then discards the returned Request, rebuilding from the original requestInit. Every header/body/method mutation performed via the Request object is silently dropped. Auth headers, tracing headers, CSRF tokens — none arrive at the server. This is a silent data-integrity regression with zero runtime signal.
On main, client-fetch already does the correct thing: it constructs request = new Request(url, requestInit) once with a real URL, loops request = await fn(request, opts), then calls _fetch(request). The interceptor's returned Request flows through untouched. The premise that this ordering is broken is incorrect for these clients — the interceptors operate on the Request, not on opts.
5. No changeset, no tests, no docs update, and load-bearing comments removed. See inline comments.
Recommended path forward: close this PR and open a targeted one-line fix against packages/openapi-ts/src/plugins/@hey-api/client-next/bundle/client.ts that moves const url = buildUrl(resolvedOpts) (currently at line 68, inside beforeRequest) out of beforeRequest and places it after the interceptors.request.fns loop in request (currently client-next/bundle/client.ts:82-86), mirroring the pattern that already exists in the same file's SSE onRequest. Add a changeset (major — public interceptor semantics) and a regression test that mutates opts.baseUrl inside an interceptor and asserts the outgoing fetch call target.
| */ | ||
|
|
||
| // 1. Create an initial Request object with a placeholder URL | ||
| let request = new Request('' as string, requestInit); |
There was a problem hiding this comment.
new Request('') throws TypeError: Failed to parse URL from in Node.js (undici) when no global origin has been set — which is the default in SSR, serverless, CLIs, and Vitest's Node environment. Because this line is inside the outer scope of request (not the try), the first call to any client method rejects with an opaque TypeError. The '' as string cast is also a no-op — it asserts the type already inferred. This pattern is unrecoverable without passing a real absolute URL.
| const url = buildUrl(opts); | ||
|
|
||
| // 4. Re-initialize the Request object with the final computed URL and original init options | ||
| request = new Request(url, requestInit); |
There was a problem hiding this comment.
The Request returned by each interceptor is assigned to request, then immediately overwritten by new Request(url, requestInit) using the original requestInit. Any header/body/method mutation an interceptor performed on the Request object (the documented auth pattern: request.headers.set('Authorization', ...); return request) is silently dropped. Only mutations routed through opts survive, which is not how any of the existing interceptor examples are written.
| * FIX (#3803): Implementing proper error interceptor threading. | ||
| * Ensure each error interceptor receives the result of the previous one. | ||
| */ | ||
| let finalError = error; |
There was a problem hiding this comment.
Threading finalError through interceptors is a real behavior change for custom-client (unlike for client-fetch/client-ky where it already landed via #3814). It needs a changeset at the major/breaking level and a migrating.md entry, mirroring the treatment given to #3814 in docs/openapi-ts/migrating.md:20-22. Please also add a regression test with at least two error interceptors asserting that the second receives the transformed value — packages/custom-client/src/__tests__/client.test.ts currently has zero interceptor tests.
| const url = buildUrl(resolvedOpts); | ||
|
|
||
| return { opts: resolvedOpts, url }; | ||
| return { opts: resolvedOpts }; |
There was a problem hiding this comment.
Dropping url from beforeRequest's return is unnecessary churn — on main, beforeRequest computes url once via buildUrl(resolvedOpts) and the result flows to both request and makeSseFn. The change loses that single-source-of-truth and forces callers to recompute buildUrl(opts) (done twice in makeSseFn after this PR, NEW lines 266 and 270). If the goal is to defer URL construction, the right move is to drop buildUrl from beforeRequest and compute it once per caller at the correct point — not delete one field and recompute it two or three times.
| }; | ||
|
|
||
| request = new Request(url, requestInit); | ||
| request = new Request('' as string, requestInit); |
There was a problem hiding this comment.
Same new Request('') runtime crash as custom-client:70. Additionally, main already has the correct pattern on this exact line: request = new Request(url, requestInit) with a real URL, followed by the interceptor loop that returns a Request, followed by _fetch(request). The interceptor's returned Request flows through unchanged — no rebuild needed, no placeholder needed. Please revert to the main pattern.
| * The final URL will be constructed after interceptors have finished | ||
| * to allow for potential mutation of opts (baseUrl, query, etc.). | ||
| */ | ||
| request = new Request('' as string, { |
There was a problem hiding this comment.
Same new Request('') runtime crash as the other clients. Additionally, as with client-fetch, main already builds a real Request here, runs interceptors, and passes the interceptor-returned Request to kyInstance(request, kyOptions). The placeholder-then-rebuild pattern is a regression, not a fix.
| body: kyOptions.body, | ||
| headers: kyOptions.headers as HeadersInit, | ||
| method: kyOptions.method, | ||
| }); |
There was a problem hiding this comment.
Rebuild discards the interceptor-returned Request (same silent-header-loss bug as client-fetch:108). Additionally asymmetric with the placeholder at line 163: the rebuild passes { body, headers, method } but the placeholder passed the full kyOptions set. Any signal, credentials, redirect, mode, etc. set on the interceptor's Request or derived from kyOptions and baked into the placeholder Request are lost. ky mitigates some of this by re-merging from kyOptions at kyInstance(request, kyOptions), but the asymmetry is a code-smell and easy to get wrong on future maintenance.
| // parseErrorResponse will run error interceptors, and re-throw when | ||
| // throwOnError is true, which bubbles already intercepted error to | ||
| // outer catch. With this flag, we can avoid outer catch running interceptors again | ||
| errorInterceptorsInvoked = true; |
There was a problem hiding this comment.
This removal dropped a load-bearing comment explaining why errorInterceptorsInvoked exists: parseErrorResponse runs error interceptors and, with throwOnError, rethrows the already-intercepted error into the outer catch, which would run interceptors a second time without this guard. The invariant is still active — errorInterceptorsInvoked = true (line 197, 289) gates the outer catch at line 293. Without the comment, a future reader seeing errorInterceptorsInvoked = true; return parseErrorResponse(...) has no explanation for the flag. Please restore.
| // run error interceptors for errors not already handled by parseErrorResponse | ||
| /** | ||
| * Error Interceptor Threading for standard caught errors. | ||
| */ |
There was a problem hiding this comment.
The removed comment (error may already be processed by parseErrorResponse, in this case we can skip running interceptors again and run error interceptors for errors not already handled by parseErrorResponse) documents the double-invocation invariant — same rationale as line 197. The new JSDoc (Error Interceptor Threading for standard caught errors) describes what the loop does, not why the guard exists, which is the non-obvious part. Please restore the original explanatory comments.
| } | ||
| } | ||
| return request; | ||
| const finalUrl = buildUrl(opts); |
There was a problem hiding this comment.
Same bugs as client-fetch:267: the interceptor's returned Request is discarded in favor of new Request(finalUrl, init) with the original init, and buildUrl(opts) is called both here (342) and in the outer url: buildUrl(opts) at 346, so the outer value is pre-interceptor and stale for reconnects.
dosubot
Bot
added
size:M
|
@aartisonigra what's the difference against #3804? |
Hi @mrlubos, thanks for pointing that out. In this PR (#3851), I've specifically focused on ensuring that buildUrl is called after all request interceptors have finished in @hey-api/client-next. This allows interceptors to dynamically mutate baseUrl, path, or query. Additionally, I've aligned the SSE logic and ensured proper error interceptor threading to stay consistent with the core client behavior. I'll check #3804 to see if there's any overlap! |
|
Also, I'm currently updating the snapshots to reflect these changes. I'll push the updates shortly once all tests are passing locally |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
hanks @SukkaW, I've just updated the snapshots and pushed the changes. All tests should be passing now |
3 participants
The Fix
This PR optimizes the request lifecycle across all core clients (client-next, client-fetch, and client-ky) to ensure interceptors work as expected.
Key Improvements:
Reactive URL: Moved buildUrl() to run after request interceptors. This allows developers to dynamically change the baseUrl, path, or query inside an interceptor.
Error Threading: Refactored the error loop so each interceptor receives the transformed error from the previous one, rather than the original raw error.
SSE Alignment: Aligns the standard HTTP flow with the existing SSE logic for better consistency.
Validation
[x] All 25/25 build tasks passed successfully (pnpm build).
[x] Verified URL mutations and error threading in local tests.
[x] Consistent implementation across all client templates.
Related Issues
Fixes #3803