Skip to content

fix: allow use of PathBasedClient with generated paths #1842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 17, 2024
Merged

fix: allow use of PathBasedClient with generated paths #1842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/little-knives-design.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"openapi-fetch": patch
---

fix: allow use of `PathBasedClient` with generated `paths`
9 changes: 3 additions & 6 deletions packages/openapi-fetch/src/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ export interface Middleware {
}

/** This type helper makes the 2nd function param required if params/requestBody are required; otherwise, optional */
export type MaybeOptionalInit<Params extends Record<HttpMethod, {}>, Location extends keyof Params> = RequiredKeysOf<
export type MaybeOptionalInit<Params, Location extends keyof Params> = RequiredKeysOf<
FetchOptions<FilterKeys<Params, Location>>
> extends never
? FetchOptions<FilterKeys<Params, Location>> | undefined
Expand All @@ -174,7 +174,7 @@ export type ClientMethod<
...init: InitParam<Init>
) => Promise<FetchResponse<Paths[Path][Method], Init, Media>>;

export type ClientForPath<PathInfo extends Record<HttpMethod, {}>, Media extends MediaType> = {
export type ClientForPath<PathInfo, Media extends MediaType> = {
[Method in keyof PathInfo as Uppercase<string & Method>]: <Init extends MaybeOptionalInit<PathInfo, Method>>(
...init: InitParam<Init>
) => Promise<FetchResponse<PathInfo[Method], Init, Media>>;
Expand Down Expand Up @@ -221,10 +221,7 @@ export default function createClient<Paths extends {}, Media extends MediaType =
clientOptions?: ClientOptions,
): Client<Paths, Media>;

export type PathBasedClient<
Paths extends Record<string, Record<HttpMethod, {}>>,
Media extends MediaType = MediaType,
> = {
export type PathBasedClient<Paths, Media extends MediaType = MediaType> = {
[Path in keyof Paths]: ClientForPath<Paths[Path], Media>;
};

Expand Down
12 changes: 12 additions & 0 deletions packages/openapi-fetch/test/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import createClient, {
type Middleware,
type MiddlewareCallbackParams,
type QuerySerializerOptions,
type Client,
type PathBasedClient,
createPathBasedClient,
} from "../src/index.js";
import { server, baseUrl, useMockRequestHandler, toAbsoluteURL } from "./fixtures/mock-server.js";
Expand Down Expand Up @@ -142,6 +144,11 @@ describe("client", () => {
}
});

it("provides a Client type", () => {
const client = createClient<paths>({ baseUrl });
expectTypeOf(client).toEqualTypeOf<Client<paths>>();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These tests are great, thank you. I’d like to have more of these in general. And since these can run inside a test-d.ts test, I’ve been thinking it would be good to separate the type vs runtime tests (and have more of these granular type assertions).

What are your thoughts on splitting runtime vs type tests? And could these be good additions to get that rolling?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an excellent question.

Libraries I've been working on so far (notably Scala.js) had to split runtime / type tests because of infrastructure restrictions (or more specifically test cases that should compile and test cases that shouldn't compile).

Personally, I often found that annoying. Because you end up with test cases like (pseudocode):

assert(max([1]) == 1)
assert(max([1,2]) == 2)

And somewhere completely different:

assertFailsCompile(max([]))

What we can do with TS is much nicer IMO:

assert(max([1]) == 1)
assert(max([1,2]) == 2)
assertThrows(
  // @ts-expect-error
  max([])
)

That all being said: IMO it does make sense to split tests by some axis (e.g. the functionality offered, independent of whether the functionality is offered by the type system or the runtime system or a combination thereof).

So in this case, it is true we are testing the Client type function and its inference (w/o inference issues, this test would not be necessary, one could simply read the return type of createClient). So this could / should indeed go into a different "describe" (and/or even a different file).

Now, along which axes to split tests (exported elements, "features", ...) is not easy. However, it is also something that can be done very ad-hoc, so there is no need to "get it right" the first time.

Splitting what is in index.test.js into multiple files is IMO definitely a good idea (merely based on the LOC count of index.test.js).

In summary:

  • Splitting tests into different categories / units / ... is IMO a good thing.
  • I'm not sure typing-time / run-time is the best splitting axis though.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In summary:

  • Splitting tests into different categories / units / ... is IMO a good thing.
  • I'm not sure typing-time / run-time is the best splitting axis though.

You’ve convinced me 🙂. Let’s split up the tests into more meaningful divisions then. I think the test suite “feels” complete currently because it’s just too big a file. But in reality is missing several cases still.

Can be a followup obviously! Thanks for weighing in.

});

describe("params", () => {
describe("path", () => {
it("typechecks", async () => {
Expand Down Expand Up @@ -1875,6 +1882,11 @@ describe("client", () => {
});

describe("path based client", () => {
it("provides a PathBasedClient type", () => {
const client = createPathBasedClient<paths>({ baseUrl });
expectTypeOf(client).toEqualTypeOf<PathBasedClient<paths>>();
});

it("performs a call without params", async () => {
const client = createPathBasedClient<paths>({ baseUrl });

Expand Down