Allow client option for custom dispatcher into fetch requests (e.g. to disable certificate validation) #1631

[mellster2012]

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

• [x] Unit tests updated
• [ ] docs/ updated (if necessary)
• [ ] pnpm run update:examples run (only applicable for openapi-typescript)

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: c9ba9dc9a5f3d971594f015895c6585bef1d1052

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[mellster2012]

@drwpow This is the redone PR which was previously at https://github.com/openapi-ts/openapi-typescript/pull/1636

[kerwanp]

Hey @mellster2012 ! Looks good to me, thanks for the PR.

This just need a changeset and a documentation update to merge this (dispatcher option).

[mellster2012]

I'll work on the improvements/suggestions as soon as time permits, including the documentation.

[mellster2012]

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.

@drwpow @kerwanp let me know what you think - thanks!

[drwpow]

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.

@drwpow @kerwanp let me know what you think - thanks!

I’m on board with trying that. Currently the problem with using the 2nd parameter approach is we found out that the fetch() implementation is different across different browsers and Node 😓. Specifically, most implementations want EITHER ✅ fetch(Request) OR ✅ fetch(string, RequestInit object), but NEVER :x: fetch(string, Request) or :x: fetch(Request, object).

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 fetch(Request) as long as we have our current middleware API that uses Request/Response for performance and minimalism. But to that end, we can adjust the middleware API too and improve that with, say, a proper RequestInit object. But one decision affects the other, and vice-versa.

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.

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.

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 fetch() call will just need a LOT of testing.

[mellster2012]

Sounds good, starting a fresh PR.

[mellster2012]

https://github.com/openapi-ts/openapi-typescript/pull/2020