Fix Request parameter being ignored by client methods

[jthacker]

Changes

This fixes a bug in openapi-fetch where the client methods would ignore the Request parameter being passed in. Previously passing in the Request parameter works when constructing the client but not if passed in via GET, POST, etc. on a constructed client.

Related to but not blocking #2224

How to Review

No impact to the API, adds tests for checking the functionality in both cases.

Checklist

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

[changeset-bot]

🦋 Changeset detected

Latest commit: f889953

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
openapi-fetch	Patch
openapi-react-query	Patch
swr-openapi	Patch

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

[netlify]

✅ Deploy Preview for openapi-ts canceled.

Name	Link
🔨 Latest commit	f889953
🔍 Latest deploy log	https://app.netlify.com/projects/openapi-ts/deploys/689b3d70f28be90008120fed

[gzm0]

Note: It seems this has been broken since the introduction of the feature: #1907

[gzm0]

Good catch and thanks for the fix. I've left some comments.

[jthacker]

@gzm0 Thanks for the feedback. Changes pushed. Now testing for all methods and removed the duplicate test for setting Request via the client constructor.

[gzm0]

Thanks for the iteration! Looks great. Just one comment about the typing in the tests.

[gzm0]

Uh, oh. With the as any, we don't actually test if the typings allow passing the { Request: SpecialRequestImplementation } (because with any anything will be allowed).

Why is it necessary?

If it is difficult to get the typing to work with test.each, it feels like it might also just test a single method.

[jthacker]

The reason I added the type assertion was due to an error about the signatures not being compatible.

This expression is not callable.
  Each member of the union type 'ClientMethod<paths, "get", `${string}/${string}`> | ClientMethod<paths, "put", `${string}/${string}`> | ClientMethod<paths, "post", `${string}/${string}`> | ClientMethod<...> | ClientMethod<...> | ClientMethod<...> | ClientMethod<...>' has signatures, but none of those signatures are compatible with each other.ts(2349)

It actually does seem to work, I get 7 failures without the fix implemented, here's a sample

 FAIL  test/common/request.test.ts > request > custom Request > uses custom Request class - PATCH
AssertionError: expected Request{} to be an instance of SpecialRequestImplementation
 ❯ customFetch test/common/request.test.ts:330:23
    328|       const customFetch = async (input: Request) => {
    329|         // make sure that the request is actually an instance of the custom request we provided
    330|         expect(input).instanceOf(SpecialRequestImplementation);
       |                       ^
    331|         return Promise.resolve(Response.json({ hello: "world" }));
    332|       };
 ❯ coreFetch src/index.js:162:26
 ❯ Object.PATCH src/index.js:283:14
 ❯ test/common/request.test.ts:339:35

I'm not an expert in TypeScript though so happy to adjust as desired. Would you prefer the more explicit non type asserted single method version? I.e. client.GET(...)? Based on the implementation this should have the same result as the current testing approach.

[gzm0]

It actually does seem to work, I get 7 failures without the fix implemented, here's a sample

Yes, but these are runtime failures. Precisely the type of thing we'd like to catch with typescript.

Would you prefer the more explicit non type asserted single method version? I.e. client.GET(...)? Based on the implementation this should have the same result as the current testing approach.

Yes, I think that would be more appropriate here. Given the implementation, testing all HTTP methods gives us marginal benefits. So I think I'd rather go with a simpler test that is closer to (expected) real-life usage.