Skip to content

Agents fix buildurl interceptor ordering #3912

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.

Sign up for GitHub

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

Open
aartisonigra wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Agents fix buildurl interceptor ordering #3912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
50be732
fix(clients): defer URL construction and thread finalError through in…
aartisonigra May 6, 2026
30050dc
chore: add changeset for interceptor fix
aartisonigra May 6, 2026
3740600
fix(client-next): defer URL construction and address review feedback
aartisonigra May 6, 2026
6d224b2
fix(clients): defer URL construction and thread finalError through in…
aartisonigra May 15, 2026
0963c7d
fix: rebuild URL after request interceptors run (#3803)
aartisonigra May 19, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/fix-interceptor-order.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@hey-api/openapi-ts": patch
---

fix(clients): defer URL construction and thread finalError through interceptors
3 changes: 3 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,6 @@ openapi-ts-error-*
# But DO NOT ignore generated snapshots!
!test/__snapshots__/test/generated
!test/__snapshots__/test/generated/**

# gstack (global install)
.amazonq/rules/gstack.md
Comment on lines +42 to +43
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personal tooling file — belongs in ~/.config/git/ignore or .git/info/exclude, not the repo's .gitignore. Please drop these three lines.

124 changes: 47 additions & 77 deletions packages/openapi-ts/src/plugins/@hey-api/client-next/bundle/client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,7 @@ export const createClient = (config: Config = {}): Client => {
};

if (opts.security) {
await setAuthParams({
...opts,
security: opts.security,
});
await setAuthParams({ ...opts, security: opts.security });
}

if (opts.requestValidator) {
Expand All @@ -59,34 +56,30 @@ export const createClient = (config: Config = {}): Client => {
opts.serializedBody = opts.bodySerializer(opts.body) as string | undefined;
}

// remove Content-Type header if body is empty to avoid sending invalid requests
if (opts.body === undefined || opts.serializedBody === '') {
opts.headers.delete('Content-Type');
}

const resolvedOpts = opts as typeof opts & ResolvedRequestOptions<ThrowOnError, Url>;
const url = buildUrl(resolvedOpts);

return { opts: resolvedOpts, url };
return {
opts: opts as typeof opts & ResolvedRequestOptions<ThrowOnError, Url>,
};
};

// @ts-expect-error
const request: Client['request'] = async (options) => {
const throwOnError = options.throwOnError ?? _config.throwOnError;

let response: Response | undefined;

try {
const { opts, url } = await beforeRequest(options);
const { opts } = await beforeRequest(options);

// request interceptors
for (const fn of interceptors.request.fns) {
if (fn) {
await fn(opts);
}
if (fn) await fn(opts);
}

// fetch must be assigned here, otherwise it would throw the error:
// TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation
const url = buildUrl(opts);
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deleted comments above this line (// fetch must be assigned here, otherwise it would throw the error: TypeError: Failed to execute 'fetch' on 'Window': Illegal invocation and // remove Content-Type header if body is empty to avoid sending invalid requests) document non-obvious gotchas. Please restore them — they're cheap maintenance signal.


const _fetch = opts.fetch!;
const requestInit: ReqInit = {
...opts,
Expand All @@ -95,15 +88,12 @@ export const createClient = (config: Config = {}): Client => {

response = await _fetch(url, requestInit);

// response interceptors
for (const fn of interceptors.response.fns) {
if (fn) {
response = await fn(response, opts);
}
if (fn) response = await fn(response, opts);
}

const result = {
response,
};
const result = { response };

if (response.ok) {
const parseAs =
Expand All @@ -112,7 +102,8 @@ export const createClient = (config: Config = {}): Client => {
: opts.parseAs) ?? 'json';

if (response.status === 204 || response.headers.get('Content-Length') === '0') {
let emptyData: any;
let emptyData: any = {};

switch (parseAs) {
case 'arrayBuffer':
case 'blob':
Expand All @@ -125,53 +116,37 @@ export const createClient = (config: Config = {}): Client => {
case 'stream':
emptyData = response.body;
break;
case 'json':
default:
emptyData = {};
break;
}
Comment on lines 105 to 119
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: removing the explicit case 'json': default: emptyData = {} in favor of an unconditional let emptyData: any = {} plus a switch with no default works for today's parseAs values, but loses the explicit default branch. If a future parseAs value is added that should not fall back to {}, this will silently misbehave. Either restore the default case or add a comment noting the fallthrough is intentional.

return {
data: emptyData,
...result,
};

return { data: emptyData, ...result };
}

let data: any;

switch (parseAs) {
case 'arrayBuffer':
case 'blob':
case 'formData':
case 'text':
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;
}

case 'stream':
return {
data: response.body,
...result,
};
return { data: response.body, ...result };
}

if (parseAs === 'json') {
if (opts.responseValidator) {
await opts.responseValidator(data);
}

if (opts.responseTransformer) {
data = await opts.responseTransformer(data);
}
if (opts.responseValidator) await opts.responseValidator(data);
if (opts.responseTransformer) data = await opts.responseTransformer(data);
}

return {
data,
...result,
};
return { data, ...result };
}

const textError = await response.text();
Expand All @@ -180,7 +155,7 @@ export const createClient = (config: Config = {}): Client => {
try {
jsonError = JSON.parse(textError);
} catch {
// noop
// ignore JSON parse error
}

throw jsonError ?? textError;
Expand All @@ -193,41 +168,35 @@ export const createClient = (config: Config = {}): Client => {
}
}

finalError = finalError || {};
if (throwOnError) throw finalError;

if (throwOnError) {
throw finalError;
}

return {
error: finalError,
response,
};
return { error: finalError || {}, response };
Comment on lines +171 to +173
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Behavior change: previously finalError = finalError || {} ran before the throwOnError branch, so a falsy finalError was always thrown as {}. Now throw finalError can throw undefined/null/0/'' if the error chain produced a falsy value. Move the || {} fallback up so both branches see it.

Suggested change
if (throwOnError) throw finalError;
if (throwOnError) {
throw finalError;
}
return {
error: finalError,
response,
};
return { error: finalError || {}, response };
finalError = finalError || {};
if (throwOnError) throw finalError;
return { error: finalError, response };

}
};

const makeMethodFn = (method: Uppercase<HttpMethod>) => (options: RequestOptions) =>
request({ ...options, method });

const makeSseFn = (method: Uppercase<HttpMethod>) => async (options: RequestOptions) => {
const { opts, url } = await beforeRequest(options);
const { opts } = await beforeRequest(options);

return createSseClient({
...opts,
body: opts.body as BodyInit | null | undefined,
method,
onRequest: async (url, init) => {
let request = new Request(url, init);
const requestInit = { ...init, url };

onRequest: async (_unusedUrl, init) => {
const clonedOpts = { ...opts };

for (const fn of interceptors.request.fns) {
if (fn) {
await fn(requestInit as ResolvedRequestOptions);
request = new Request(requestInit.url, requestInit);
}
if (fn) await fn(clonedOpts);
}
return request;

const finalizedUrl = buildUrl(clonedOpts);
return new Request(finalizedUrl, init);
Comment on lines +188 to +196
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interceptor mutations to headers, body, signal, etc. are dropped here: clonedOpts is fed to buildUrl, but the init passed to new Request is the original requestInit from createSseClient and never sees the mutations. The previous shape (single requestInit object that the loop mutated, then new Request(requestInit.url, requestInit)) handled this correctly — please restore that pattern, just with buildUrl invoked once after the loop instead of relying on a pre-built url.

},

serializedBody: getValidRequestBody(opts) as BodyInit | null | undefined,
url,
});
};

Expand All @@ -236,6 +205,7 @@ export const createClient = (config: Config = {}): Client => {
return {
buildUrl: _buildUrl,
connect: makeMethodFn('CONNECT'),

Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray blank line inside the returned object literal — breaks the alphabetical clustering of method keys.

Suggested change
delete: makeMethodFn('DELETE'),

delete: makeMethodFn('DELETE'),
get: makeMethodFn('GET'),
getConfig,
Expand All @@ -249,15 +219,15 @@ export const createClient = (config: Config = {}): Client => {
setConfig,
sse: {
connect: makeSseFn('CONNECT'),
delete: makeSseFn('DELETE'),
get: makeSseFn('GET'),
head: makeSseFn('HEAD'),
options: makeSseFn('OPTIONS'),
patch: makeSseFn('PATCH'),
post: makeSseFn('POST'),
put: makeSseFn('PUT'),
trace: makeSseFn('TRACE'),
delete: makeMethodFn('DELETE'),
get: makeMethodFn('GET'),
head: makeMethodFn('HEAD'),
options: makeMethodFn('OPTIONS'),
patch: makeMethodFn('PATCH'),
post: makeMethodFn('POST'),
put: makeMethodFn('PUT'),
trace: makeMethodFn('TRACE'),
Comment on lines +222 to +229
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regression: every SSE method except connect is now wired to makeMethodFn, which returns a normal HTTP response instead of an SSE stream. Callers of client.sse.get/post/... will silently break — the as unknown as Client cast at the bottom of the file hides the resulting type mismatch. Restore makeSseFn for these entries.

Suggested change
delete: makeMethodFn('DELETE'),
get: makeMethodFn('GET'),
head: makeMethodFn('HEAD'),
options: makeMethodFn('OPTIONS'),
patch: makeMethodFn('PATCH'),
post: makeMethodFn('POST'),
put: makeMethodFn('PUT'),
trace: makeMethodFn('TRACE'),
delete: makeSseFn('DELETE'),
get: makeSseFn('GET'),
head: makeSseFn('HEAD'),
options: makeSseFn('OPTIONS'),
patch: makeSseFn('PATCH'),
post: makeSseFn('POST'),
put: makeSseFn('PUT'),
trace: makeSseFn('TRACE'),

},
trace: makeMethodFn('TRACE'),
} as Client;
} as unknown as Client;
Copy link
Copy Markdown
Contributor

@pullfrog pullfrog Bot May 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as unknown as Client is a code smell here — it's masking the type mismatch introduced by routing SSE methods through makeMethodFn. Once the SSE map is restored, this should go back to as Client.

};