Allow client option for custom dispatcher into fetch requests (e.g. to disable certificate validation) #1631 #1636
Conversation
|
|
Is there anything we can do to re-run failed workflows? Thanks! |
Unfortunately no; there’s just a requirement that new contributors need to have manually-run workflows until they’ve gotten a PR merged. I think it’s just GitHub saving $ (which is fine by me, because of how generously they provide so many resources for OSS). |
|
See comment on #1631. I think we need a little more discussion / clarification on what this PR is accomplishing, because on first glance it’s not clear. So that’s priority 1. I’ll also leave a few comments on this PR—even if it gets reworked—just to give a sense of what we’re looking for when contributing to this project. |
|
|
||
| afterAll(() => server.close()); | ||
|
|
||
| describe("TypeScript checks", () => { |
There was a problem hiding this comment.
Appreciate you adding so many test cases to this PR, but we don’t want to just flat-out copy the entire test suite just for one change. I would suspect most of the tests don’t actually test this code path at all; we probably only need 1 or 2 new tests to start.
There may be some confusion with the v7-beta.test.ts file; that’s a separate case. It’s because when openapi-typescript 7.x gets a stable release, we’ll have to make a breaking release for openapi-fetch as well, and I’m just making sure that transition is smooth. But again, for a single option, we don’t need to copy all the tests
There was a problem hiding this comment.
Removed the extra test file and just added one test under options specifying a custom dispatcher - without it hitting a 'bad cert' website it only validates that specifying a custom dispatcher does not break general functionality.
package.json
Outdated
| @@ -20,6 +20,7 @@ | |||
| "@changesets/cli": "^2.27.1", | |||
| "del-cli": "^5.1.0", | |||
| "prettier": "^3.2.5", | |||
| "typescript": "^5.4.5" | |||
| "typescript": "^5.4.5", | |||
| "undici": "^6.14.1" | |||
There was a problem hiding this comment.
We’re only using undici in a single test in openapi-fetch, so we want to add it to devDependencies in that package.json; not for the entire monorepo.
| @@ -15,6 +15,8 @@ import type { | |||
| export interface ClientOptions extends Omit<RequestInit, "headers"> { | |||
| /** set the common root URL for all API requests */ | |||
| baseUrl?: string; | |||
| /** custom dispatcher */ | |||
| dispatcher?: unknown; | |||
There was a problem hiding this comment.
We’ll want a more accurate type for this.
|
any chance of this getting merged? i'm getting hit with this problem with a long-running fetch call (undici hardcodes the timeout when used through the fetch api) |
|
Apologies; I dropped off reviewing this PR because it still had failing tests, and it seemed like there was still work to be done. @mellster2012 if we get all the tests and checks passing again, I’d be open to approving and merging 🙂. Thanks for addressing my previous comments, by the way! |
Sure, I will definitely work on it and resubmit once everything is back in sync and tests are passing, but it may take a week or more until I can get started on it. |
|
Redone PR here: #1837 |
Changes
Fix for #1631
How to Review
How can a reviewer review your changes? What should be kept in mind for this review?
Checklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)