-
-
Notifications
You must be signed in to change notification settings - Fork 501
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
feat(openapi-fetch): Allow returning Response from onRequest callback #2091
base: main
Are you sure you want to change the base?
Changes from all commits
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 |
---|---|---|
|
@@ -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!