-
-
Notifications
You must be signed in to change notification settings - Fork 373
fix(clients): defer URL construction and thread finalError through interceptors (#3803) #3851
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
50be732
30050dc
3740600
6d224b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -55,20 +55,33 @@ | |
| opts.headers.delete('Content-Type'); | ||
| } | ||
|
|
||
| const url = buildUrl(opts); | ||
| const requestInit: ReqInit = { | ||
| redirect: 'follow', | ||
| ...opts, | ||
| }; | ||
|
|
||
| let request = new Request(url, requestInit); | ||
| /** | ||
| * FIX (#3803): Execute request interceptors before building the final URL. | ||
| * This ensures that any mutations made to 'opts' (e.g., baseUrl, path, query) | ||
| * by the interceptors are correctly captured during the URL construction phase. | ||
| */ | ||
|
|
||
| // 1. Create an initial Request object with a placeholder URL | ||
| let request = new Request('' as string, requestInit); | ||
|
Check failure on line 70 in packages/custom-client/src/client.ts
|
||
|
|
||
| // 2. Process all registered request interceptors | ||
| for (const fn of interceptors.request.fns) { | ||
| if (fn) { | ||
| request = await fn(request, opts); | ||
| } | ||
| } | ||
|
|
||
| // 3. Construct the final URL using the potentially modified options | ||
| const url = buildUrl(opts); | ||
|
|
||
| // 4. Re-initialize the Request object with the final computed URL and original init options | ||
| request = new Request(url, requestInit); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
|
|
||
| // fetch must be assigned here, otherwise it would throw the error: | ||
| // TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation | ||
| const _fetch = opts.fetch!; | ||
|
|
@@ -107,8 +120,6 @@ | |
| data = await response[parseAs](); | ||
| break; | ||
| case 'json': { | ||
| // Some servers return 200 with no Content-Length and empty body. | ||
| // response.json() would throw; read as text and parse if non-empty. | ||
| const text = await response.text(); | ||
| data = text ? JSON.parse(text) : {}; | ||
| break; | ||
|
|
@@ -143,6 +154,10 @@ | |
| // noop | ||
| } | ||
|
|
||
| /** | ||
| * FIX (#3803): Implementing proper error interceptor threading. | ||
| * Ensure each error interceptor receives the result of the previous one. | ||
| */ | ||
| let finalError = error; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Threading |
||
|
|
||
| for (const fn of interceptors.error.fns) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,9 +67,8 @@ export const createClient = (config: Config = {}): Client => { | |
|
|
||
| const resolvedOpts = opts as typeof opts & | ||
| ResolvedRequestOptions<TResponseStyle, ThrowOnError, Url>; | ||
| const url = buildUrl(resolvedOpts); | ||
|
|
||
| return { opts: resolvedOpts, url }; | ||
| return { opts: resolvedOpts }; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Dropping |
||
| }; | ||
|
|
||
| const request: Client['request'] = async (options) => { | ||
|
|
@@ -80,21 +79,34 @@ export const createClient = (config: Config = {}): Client => { | |
| let response: Response | undefined; | ||
|
|
||
| try { | ||
| const { opts, url } = await beforeRequest(options); | ||
| const { opts } = await beforeRequest(options); | ||
|
|
||
| /** | ||
| * Initialize request object with a placeholder URL. | ||
| * The final URL will be constructed after interceptors have finished | ||
| * to allow for potential mutation of opts (baseUrl, query, etc.). | ||
| */ | ||
| const requestInit: ReqInit = { | ||
| redirect: 'follow', | ||
| ...opts, | ||
| body: getValidRequestBody(opts), | ||
| }; | ||
|
|
||
| request = new Request(url, requestInit); | ||
| request = new Request('' as string, requestInit); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
|
|
||
| // 1. Process all request interceptors | ||
| for (const fn of interceptors.request.fns) { | ||
| if (fn) { | ||
| request = await fn(request, opts); | ||
| } | ||
| } | ||
|
|
||
| // 2. Build final URL after interceptors have potentially mutated options | ||
| const url = buildUrl(opts); | ||
|
|
||
| // 3. Re-initialize Request with the finalized computed URL | ||
| request = new Request(url, requestInit); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rebuild discards the Request returned by the interceptor loop. The canonical documented pattern ( |
||
|
|
||
| // fetch must be assigned here, otherwise it would throw the error: | ||
| // TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation | ||
| const _fetch = opts.fetch!; | ||
|
|
@@ -198,6 +210,11 @@ export const createClient = (config: Config = {}): Client => { | |
|
|
||
| throw jsonError ?? textError; | ||
| } catch (error) { | ||
| /** | ||
| * Implementation of error interceptor threading. | ||
| * Ensures that each interceptor in the chain receives the processed error | ||
| * from the previous one. | ||
| */ | ||
| let finalError = error; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The loop body below this comment is byte-identical to |
||
|
|
||
| for (const fn of interceptors.error.fns) { | ||
|
|
@@ -227,22 +244,30 @@ export const createClient = (config: Config = {}): Client => { | |
| request({ ...options, method }); | ||
|
|
||
| const makeSseFn = (method: Uppercase<HttpMethod>) => async (options: RequestOptions) => { | ||
| const { opts, url } = await beforeRequest(options); | ||
| const { opts } = await beforeRequest(options); | ||
|
|
||
| /** | ||
| * SSE Implementation: Defer URL construction to ensure onRequest | ||
| * interceptors can properly mutate the request flow. | ||
| */ | ||
| return createSseClient({ | ||
| ...opts, | ||
| body: opts.body as BodyInit | null | undefined, | ||
| method, | ||
| onRequest: async (url, init) => { | ||
| let request = new Request(url, init); | ||
| onRequest: async (initialUrl, init) => { | ||
| let request = new Request(initialUrl, init); | ||
| for (const fn of interceptors.request.fns) { | ||
| if (fn) { | ||
| request = await fn(request, opts); | ||
| } | ||
| } | ||
| return request; | ||
|
|
||
| // Re-build final URL after interceptors to capture mutations | ||
| const finalUrl = buildUrl(opts); | ||
| return new Request(finalUrl, init); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Two issues: (1) the |
||
| }, | ||
| serializedBody: getValidRequestBody(opts) as BodyInit | null | undefined, | ||
| url, | ||
| url: buildUrl(opts), | ||
| }); | ||
| }; | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,7 +41,6 @@ export const createClient = (config: Config = {}): Client => { | |
| ...options, | ||
| headers: mergeHeaders(_config.headers, options.headers), | ||
| ky: options.ky ?? _config.ky ?? ky, | ||
| // deep merge kyOptions to ensure base _config is being respected | ||
| kyOptions: { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment ( |
||
| ..._config.kyOptions, | ||
| ...options.kyOptions, | ||
|
|
@@ -70,9 +69,8 @@ export const createClient = (config: Config = {}): Client => { | |
|
|
||
| const resolvedOpts = opts as typeof opts & | ||
| ResolvedRequestOptions<TResponseStyle, ThrowOnError, Url>; | ||
| const url = buildUrl(resolvedOpts); | ||
|
|
||
| return { opts: resolvedOpts, url }; | ||
| return { opts: resolvedOpts }; | ||
| }; | ||
|
|
||
| const parseErrorResponse = async ( | ||
|
|
@@ -96,6 +94,12 @@ export const createClient = (config: Config = {}): Client => { | |
| } | ||
|
|
||
| const error = jsonError ?? textError; | ||
|
|
||
| /** | ||
| * Implementation of error interceptor threading. | ||
| * Ensures that each interceptor in the chain receives the processed error | ||
| * from the previous one. | ||
| */ | ||
| let finalError = error; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as |
||
|
|
||
| for (const fn of interceptorsMiddleware.error.fns) { | ||
|
|
@@ -127,10 +131,9 @@ export const createClient = (config: Config = {}): Client => { | |
| let errorInterceptorsInvoked = false; | ||
|
|
||
| try { | ||
| const { opts, url } = await beforeRequest(options); | ||
| const { opts } = await beforeRequest(options); | ||
|
|
||
| const kyInstance = opts.ky!; | ||
|
|
||
| const validBody = getValidRequestBody(opts); | ||
|
|
||
| const kyOptions: KyOptions = { | ||
|
|
@@ -152,7 +155,12 @@ export const createClient = (config: Config = {}): Client => { | |
| retry: opts.retry ?? opts.kyOptions?.retry ?? 2, | ||
| }; | ||
|
|
||
| request = new Request(url, { | ||
| /** | ||
| * Initialize request object with a placeholder URL. | ||
| * 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, { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same |
||
| body: kyOptions.body, | ||
| headers: kyOptions.headers as HeadersInit, | ||
| method: kyOptions.method, | ||
|
|
@@ -164,6 +172,16 @@ export const createClient = (config: Config = {}): Client => { | |
| } | ||
| } | ||
|
|
||
| // Re-build final URL after interceptors to capture mutations | ||
| const url = buildUrl(opts); | ||
|
|
||
| // Re-initialize Request with the finalized computed URL | ||
| request = new Request(url, { | ||
| body: kyOptions.body, | ||
| headers: kyOptions.headers as HeadersInit, | ||
| method: kyOptions.method, | ||
| }); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rebuild discards the interceptor-returned Request (same silent-header-loss bug as |
||
|
|
||
| try { | ||
| response = await kyInstance(request, kyOptions); | ||
| } catch (error) { | ||
|
|
@@ -176,9 +194,6 @@ export const createClient = (config: Config = {}): Client => { | |
| } | ||
| } | ||
|
|
||
| // 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; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This removal dropped a load-bearing comment explaining why |
||
| return parseErrorResponse(response, request, opts, interceptors); | ||
| } | ||
|
|
@@ -239,8 +254,6 @@ export const createClient = (config: Config = {}): Client => { | |
| data = await response[parseAs](); | ||
| break; | ||
| case 'json': { | ||
| // Some servers return 200 with no Content-Length and empty body. | ||
| // response.json() would throw; read as text and parse if non-empty. | ||
| const text = await response.text(); | ||
| data = text ? JSON.parse(text) : {}; | ||
| break; | ||
|
|
@@ -272,18 +285,15 @@ export const createClient = (config: Config = {}): Client => { | |
| }; | ||
| } | ||
|
|
||
| // 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; | ||
| return parseErrorResponse(response, request, opts, interceptors); | ||
| } catch (error) { | ||
| let finalError = error; | ||
|
|
||
| // error may already be processed by parseErrorResponse, in this case | ||
| // we can skip running interceptors again | ||
| if (!errorInterceptorsInvoked) { | ||
| // run error interceptors for errors not already handled by parseErrorResponse | ||
| /** | ||
| * Error Interceptor Threading for standard caught errors. | ||
| */ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The removed comment ( |
||
| for (const fn of interceptors.error.fns) { | ||
| if (fn) { | ||
| finalError = await fn(finalError, response, request, options as ResolvedRequestOptions); | ||
|
|
@@ -311,23 +321,29 @@ export const createClient = (config: Config = {}): Client => { | |
| request({ ...options, method }); | ||
|
|
||
| const makeSseFn = (method: Uppercase<HttpMethod>) => async (options: RequestOptions) => { | ||
| const { opts, url } = await beforeRequest(options); | ||
| const { opts } = await beforeRequest(options); | ||
|
|
||
| /** | ||
| * SSE Implementation: Defer URL construction to ensure onRequest | ||
| * interceptors can properly mutate the request flow. | ||
| */ | ||
| return createSseClient({ | ||
| ...opts, | ||
| body: opts.body as BodyInit | null | undefined, | ||
| fetch: globalThis.fetch, | ||
| method, | ||
| onRequest: async (url, init) => { | ||
| let request = new Request(url, init); | ||
| onRequest: async (initialUrl, init) => { | ||
| let request = new Request(initialUrl, init); | ||
| for (const fn of interceptors.request.fns) { | ||
| if (fn) { | ||
| request = await fn(request, opts); | ||
| } | ||
| } | ||
| return request; | ||
| const finalUrl = buildUrl(opts); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same bugs as |
||
| return new Request(finalUrl, init); | ||
| }, | ||
| serializedBody: getValidRequestBody(opts) as BodyInit | null | undefined, | ||
| url, | ||
| url: buildUrl(opts), | ||
| }); | ||
| }; | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new Request('')throwsTypeError: Failed to parse URL fromin 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 ofrequest(not thetry), the first call to any client method rejects with an opaqueTypeError. The'' as stringcast is also a no-op — it asserts the type already inferred. This pattern is unrecoverable without passing a real absolute URL.