feat(openapi-fetch): Allow returning Response from onRequest callback #2091
Conversation
👷 Deploy request for openapi-ts pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: fad2a50 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
p-dubovitsky
force-pushed
the
early-response
branch
from
3532cdc to
fad2a50
Compare
| onResponse({ request, response }) { | ||
| if (response.ok) { | ||
| const key = getCacheKey(request); | ||
| cache.set(key, response); |
There was a problem hiding this comment.
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:
clone() throws a TypeError if the response body has already been used.
(https://developer.mozilla.org/en-US/docs/Web/API/Response/clone)
There was a problem hiding this comment.
Yes, I think you're right. Thanks for your consideration!
| 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.
I think this error message was wrong an now it feels even more wrong :-/ (IIUC you must not return new Request() but mutate the request in place and return it, unclear to me why returning the request is even necessary...).
Probably not blocking for this PR, but a fix would be appreciated.
There was a problem hiding this comment.
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.
I'm not sure I understood you correctly. Do you want to throw an error without a message or something else?
|
|
||
| test("can return response directly from onRequest", async () => { | ||
| const customResponse = Response.json({}); | ||
| const client = createObservedClient<paths>(); |
There was a problem hiding this comment.
| const client = createObservedClient<paths>(); | |
| const client = createObservedClient<paths>({}, () => throw new Error("unexpected call to fetch")); |
This way, we make sure we really do not call fetch.
|
|
||
| test("skips subsequent onRequest handlers when response is returned", async () => { | ||
| let onRequestCalled = false; | ||
| const customResponse = Response.json({}); |
There was a problem hiding this comment.
Nit: Inline below? You don't actually need the reference for this test.
|
|
||
| test("skips onResponse handlers when response is returned from onRequest", async () => { | ||
| let onResponseCalled = false; | ||
| const customResponse = Response.json({}); |
Changes
This PR closes #2072 by adding ability to return
ResponsefromonRequestmiddleware hook. This allows middleware to short-circuit the request chain by returning a response directly, which is useful for cases such as deduplicating or caching responses to avoid unnecessary network requests.How to Review
Please check that:
ResponsefromonRequestproperly skips remaining middleware chainKey implementation details:
Responseis returned:onRequesthandlers are skippedonResponsehandlers are skippedChecklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)