Allow client option for custom dispatcher into fetch requests (e.g. to disable certificate validation) #1631 #1837
Conversation
|
|
Hey @mellster2012 ! Looks good to me, thanks for the PR. This just need a changeset and a documentation update to merge this (dispatcher option). |
| @@ -10,10 +10,14 @@ import type { | |||
| SuccessResponse, | |||
| } from "openapi-typescript-helpers"; | |||
|
|
|||
| import type { Dispatcher } from "undici"; | |||
There was a problem hiding this comment.
Can we redeclare this type so we don’t have an undici dependency (even type-wise)? The type signatures can match, but I don’t want people who install openapi-fetch to also have undici install as well.
packages/openapi-fetch/package.json
Outdated
| @@ -77,6 +77,7 @@ | |||
| "openapi-typescript-codegen": "^0.25.0", | |||
| "openapi-typescript-fetch": "^2.0.0", | |||
| "superagent": "^9.0.2", | |||
| "undici": "^6.14.1", | |||
There was a problem hiding this comment.
See other comment—unfortunately this is a dependency because of how it’s used, and I’d like to avoid that. Let’s just copy the Dispatcher type locally to prevent this.
packages/openapi-fetch/src/index.js
Outdated
| @@ -125,7 +126,7 @@ export default function createClient(clientOptions) { | |||
| } | |||
|
|
|||
| // fetch! | |||
| let response = await fetch(request); | |||
| let response = await fetch(request, dispatcher ? { dispatcher } : undefined); | |||
There was a problem hiding this comment.
Note: I’m a bit skeptical of this as-written, because this is invalid for the official fetch() specification. It doesn’t throw an error just by virtue of extra params getting ignored, but in basically all implementations of fetch() that aren’t undici, this is invalid.
Of course, the argument I guess is people won’t be adding this if they aren’t using undici as their custom fetcher, so it’s at least within the user’s control. But I want to point out that treating all fetch()s as if they’re a custom third-party implementation is dangerous (I know undici is basically used as Node’s official fetch() implementation now, but it still differs in significant ways from browsers so we have to be careful).
Not a hard requirement, but it would be nice to have an additional check here, such as dispatcher && isUndici ? … where something is determined inside createClient() to check if the fetch() function signature will accept this param or not.
There was a problem hiding this comment.
I’m still in favor of adding this feature, and this is getting there, but we’re missing documentation! We need to answer:
- When would users use this option? What usecases/prior art? (We chatted about this in another issue, so just consolidating that into docs so others can find that info)
- How would they opt in? Do they need to use undici? Will other custom fetch clients work?
- What is some example code they can copy/paste to get started with this feature?
All of this should be clear to users how to use this function.
Also be sure to add a patch changeset to openapi-fetch (yes this is a feature add, but we’re following the convention of minor changes are breaking in 0.x, and this isn’t breaking, I believe)!
| await client.GET("/self"); | ||
|
|
||
| // assert baseUrl and path mesh as expected | ||
| expect(getRequestUrl().href).toBe(toAbsoluteURL("/self")); |
There was a problem hiding this comment.
Is this test testing what we think it is? I don’t see how this uses dispatch (or maybe I’m missing it?)
|
I'll work on the improvements/suggestions as soon as time permits, including the documentation. |
|
The Dispatcher type is quite large and contains other (nested) undici custom types, and including all these extra types doesn't provide much benefit for the bloat. Also the fetch implementation here by default does not provide an optional RequestInit, rather an expanded requestInit is hidden within the CustomRequest type, which seems already like a deviation. My suggestion would be to revert back to providing the optional addition of custom RequestInit properties into the ClientOptions, e.g. RequestInitExtensions of type Record<string, unknown>, so any number of custom properties can be added, and pass them to the fetch call as 2nd parameter as RequestInit when present (like it's currently proposed in this PR), with the user assuming the control and responsibility of using the correct custom fetch implementation with it. |
I’m on board with trying that. Currently the problem with using the 2nd parameter approach is we found out that the Fortunately we added tests to catch this that does catch some gamebreaking regressions from happening again, but taking a 2nd look over everything I’m not confident we have all the tests in place (will work on adding that). All that said, for now, we’re somewhat stuck with the single-param fetch API of Sorry for the extra context; this just overlaps several other APIs/behavior, and past regressions, so I wanted to spend a little more thought on it.
To previous point, the CustomRequest type extends the built-in Request class, and though it does allow for additional properties, is still a valid implementation of the fetch spec that works in Node.js and all browsers, and is tested and working. So any changes to the |
|
Sounds good, starting a fresh PR. |
Changes
Allow client option for custom dispatcher into fetch requests (e.g. to disable certificate validation)
How to Review
This has been discussed before under a previous PR #1636. The branches were quite out of sync so it was easier to start from scratch.
Checklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)