-
-
Notifications
You must be signed in to change notification settings - Fork 529
feat(openapi-fetch): Allow returning Response from onRequest callback #2091
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
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 |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"openapi-fetch": patch | ||
--- | ||
|
||
Allow returning Response from onRequest callback |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -64,6 +64,38 @@ onRequest({ schemaPath }) { | |
|
||
This will leave the request/response unmodified, and pass things off to the next middleware handler (if any). There’s no internal callback or observer library needed. | ||
|
||
### Early Response | ||
|
||
You can return a `Response` directly from `onRequest`, which will skip the actual request and remaining middleware chain. This is useful for cases such as deduplicating or caching responses to avoid unnecessary network requests. | ||
|
||
```ts | ||
const cache = new Map<string, Response>(); | ||
const getCacheKey = (request: Request) => `${request.method}:${request.url}`; | ||
|
||
const cacheMiddleware: Middleware = { | ||
onRequest({ request }) { | ||
const key = getCacheKey(request); | ||
const cached = cache.get(key); | ||
if (cached) { | ||
// Return cached response, skipping actual request and remaining middleware chain | ||
return cached.clone(); | ||
} | ||
}, | ||
onResponse({ request, response }) { | ||
if (response.ok) { | ||
const key = getCacheKey(request); | ||
cache.set(key, response); | ||
} | ||
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. Here you must return 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. Are you sure? The response in the cache is cloned. I say this only from looking at the code, if you have tried this and it fails, well, then it fails :) PR to fix the example in this case is very much appreciated. 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. I am using nextjs 15 (server-side requests). At some point const cache = new Map<string, string>();
const getCacheKey = (request: Request) => `${request.method}:${request.url}`;
const cacheMiddleware: Middleware = {
onRequest({ request }) {
const key = getCacheKey(request);
const cached = cache.get(key);
if (cached) {
// Return cached response, skipping actual request and remaining middleware chain
const response = new Response(cached);
return response;
}
},
async onResponse({ request, response }) {
if (response.ok) {
const key = getCacheKey(request);
const responseBody = await response.clone().text();
cache.set(key, responseBody);
}
}
}; 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. TY for the example. Unfortunately, without more understanding of what is going on here, from the maintainers side, I don't think we have a strong enough signal to take this further. If you would like, please file a bug with this, and investigation can proceed there. However, please understand that you will be expected to produce a reproducible example that relies on a (standard) fetch implementation only (of course that needs to run in browsers and Node.js, but no SSR magic, just free-standing code). |
||
} | ||
}; | ||
``` | ||
|
||
When a middleware returns a `Response`: | ||
|
||
* The request is not sent to the server | ||
* Subsequent `onRequest` handlers are skipped | ||
* `onResponse` handlers are skipped | ||
|
||
### Throwing | ||
|
||
Middleware can also be used to throw an error that `fetch()` wouldn’t normally, useful in libraries like [TanStack Query](https://tanstack.com/query/latest): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,6 +95,7 @@ export default function createClient(clientOptions) { | |
let id; | ||
let options; | ||
let request = new CustomRequest(createFinalURL(schemaPath, { baseUrl, params, querySerializer }), requestInit); | ||
let response; | ||
|
||
/** Add custom parameters to Request object */ | ||
for (const key in init) { | ||
|
@@ -124,79 +125,84 @@ export default function createClient(clientOptions) { | |
id, | ||
}); | ||
if (result) { | ||
if (!(result instanceof CustomRequest)) { | ||
throw new Error("onRequest: must return new Request() when modifying the request"); | ||
if (result instanceof CustomRequest) { | ||
request = result; | ||
} else if (result instanceof Response) { | ||
response = result; | ||
break; | ||
} else { | ||
throw new Error("onRequest: must return new Request() or Response() when modifying the request"); | ||
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. I think this error message was wrong an now it feels even more wrong :-/ (IIUC you must not return Probably not blocking for this PR, but a fix would be appreciated. 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. Yeah I’m keen to just remove this (not necessarily in this PR, just in general); this is not true in some cases. It was meant to provide a friendlier error for folks, but I actually think just letting the platform surface the error is more helpful, rather than us getting in the way 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. I'm not sure I understood you correctly. Do you want to throw an error without a message or something else? |
||
} | ||
request = result; | ||
} | ||
} | ||
} | ||
} | ||
|
||
// fetch! | ||
let response; | ||
try { | ||
response = await fetch(request, requestInitExt); | ||
} catch (error) { | ||
let errorAfterMiddleware = error; | ||
// middleware (error) | ||
if (!response) { | ||
// fetch! | ||
try { | ||
response = await fetch(request, requestInitExt); | ||
} catch (error) { | ||
let errorAfterMiddleware = error; | ||
// middleware (error) | ||
// execute in reverse-array order (first priority gets last transform) | ||
if (middlewares.length) { | ||
for (let i = middlewares.length - 1; i >= 0; i--) { | ||
const m = middlewares[i]; | ||
if (m && typeof m === "object" && typeof m.onError === "function") { | ||
const result = await m.onError({ | ||
request, | ||
error: errorAfterMiddleware, | ||
schemaPath, | ||
params, | ||
options, | ||
id, | ||
}); | ||
if (result) { | ||
// if error is handled by returning a response, skip remaining middleware | ||
if (result instanceof Response) { | ||
errorAfterMiddleware = undefined; | ||
response = result; | ||
break; | ||
} | ||
|
||
if (result instanceof Error) { | ||
errorAfterMiddleware = result; | ||
continue; | ||
} | ||
|
||
throw new Error("onError: must return new Response() or instance of Error"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// rethrow error if not handled by middleware | ||
if (errorAfterMiddleware) { | ||
throw errorAfterMiddleware; | ||
} | ||
} | ||
|
||
// middleware (response) | ||
// execute in reverse-array order (first priority gets last transform) | ||
if (middlewares.length) { | ||
for (let i = middlewares.length - 1; i >= 0; i--) { | ||
const m = middlewares[i]; | ||
if (m && typeof m === "object" && typeof m.onError === "function") { | ||
const result = await m.onError({ | ||
if (m && typeof m === "object" && typeof m.onResponse === "function") { | ||
const result = await m.onResponse({ | ||
request, | ||
error: errorAfterMiddleware, | ||
response, | ||
schemaPath, | ||
params, | ||
options, | ||
id, | ||
}); | ||
if (result) { | ||
// if error is handled by returning a response, skip remaining middleware | ||
if (result instanceof Response) { | ||
errorAfterMiddleware = undefined; | ||
response = result; | ||
break; | ||
if (!(result instanceof Response)) { | ||
throw new Error("onResponse: must return new Response() when modifying the response"); | ||
} | ||
|
||
if (result instanceof Error) { | ||
errorAfterMiddleware = result; | ||
continue; | ||
} | ||
|
||
throw new Error("onError: must return new Response() or instance of Error"); | ||
} | ||
} | ||
} | ||
} | ||
|
||
// rethrow error if not handled by middleware | ||
if (errorAfterMiddleware) { | ||
throw errorAfterMiddleware; | ||
} | ||
} | ||
|
||
// middleware (response) | ||
// execute in reverse-array order (first priority gets last transform) | ||
if (middlewares.length) { | ||
for (let i = middlewares.length - 1; i >= 0; i--) { | ||
const m = middlewares[i]; | ||
if (m && typeof m === "object" && typeof m.onResponse === "function") { | ||
const result = await m.onResponse({ | ||
request, | ||
response, | ||
schemaPath, | ||
params, | ||
options, | ||
id, | ||
}); | ||
if (result) { | ||
if (!(result instanceof Response)) { | ||
throw new Error("onResponse: must return new Response() when modifying the response"); | ||
response = result; | ||
} | ||
response = result; | ||
} | ||
} | ||
} | ||
|
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. Nice tests! 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. Thanks! |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -443,3 +443,64 @@ test("type error occurs only when neither onRequest nor onResponse is specified" | |||||
assertType<Middleware>({ onResponse }); | ||||||
assertType<Middleware>({ onRequest, onResponse }); | ||||||
}); | ||||||
|
||||||
test("can return response directly from onRequest", async () => { | ||||||
const customResponse = Response.json({}); | ||||||
const client = createObservedClient<paths>(); | ||||||
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.
Suggested change
This way, we make sure we really do not call fetch. |
||||||
|
||||||
client.use({ | ||||||
async onRequest() { | ||||||
return customResponse; | ||||||
}, | ||||||
}); | ||||||
|
||||||
const { response } = await client.GET("/posts/{id}", { | ||||||
params: { path: { id: 123 } }, | ||||||
}); | ||||||
|
||||||
expect(response).toBe(customResponse); | ||||||
}); | ||||||
|
||||||
test("skips subsequent onRequest handlers when response is returned", async () => { | ||||||
let onRequestCalled = false; | ||||||
const customResponse = Response.json({}); | ||||||
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. Nit: Inline below? You don't actually need the reference for this test. |
||||||
const client = createObservedClient<paths>(); | ||||||
|
||||||
client.use( | ||||||
{ | ||||||
async onRequest() { | ||||||
return customResponse; | ||||||
}, | ||||||
}, | ||||||
{ | ||||||
async onRequest() { | ||||||
onRequestCalled = true; | ||||||
return undefined; | ||||||
}, | ||||||
}, | ||||||
); | ||||||
|
||||||
await client.GET("/posts/{id}", { params: { path: { id: 123 } } }); | ||||||
|
||||||
expect(onRequestCalled).toBe(false); | ||||||
}); | ||||||
|
||||||
test("skips onResponse handlers when response is returned from onRequest", async () => { | ||||||
let onResponseCalled = false; | ||||||
const customResponse = Response.json({}); | ||||||
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: Inline below? |
||||||
const client = createObservedClient<paths>(); | ||||||
|
||||||
client.use({ | ||||||
async onRequest() { | ||||||
return customResponse; | ||||||
}, | ||||||
async onResponse() { | ||||||
onResponseCalled = true; | ||||||
return undefined; | ||||||
}, | ||||||
}); | ||||||
|
||||||
await client.GET("/posts/{id}", { params: { path: { id: 123 } } }); | ||||||
|
||||||
expect(onResponseCalled).toBe(false); | ||||||
}); |
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.
Are you sure we do not need to clone the response here? The response we return is (probably) going to be consumed by the caller.
IIUC,
clone()
will throw if the response has already been used:(https://developer.mozilla.org/en-US/docs/Web/API/Response/clone)
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.
Yes, I think you're right. Thanks for your consideration!