fix: allow use of PathBasedClient with generated paths
#1842
Conversation
🦋 Changeset detectedLatest commit: a88ee19 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
To achieve this, we remove unnecessary type bounds. This fixes openapi-ts#1840.
| @@ -142,6 +144,11 @@ describe("client", () => { | |||
| } | |||
| }); | |||
|
|
|||
| it("provides a Client type", () => { | |||
| const client = createClient<paths>({ baseUrl }); | |||
| expectTypeOf(client).toEqualTypeOf<Client<paths>>(); | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Great work! In favor of this merging, but see comment on type vs runtime tests |
|
For posterity: I have been working around this using an intersection type: // BAD!!! See explanation below.
type Client = PathBasedClient<paths & Record<string, Record<string, string>>>;However, this breaks inference on usage-site for wrong paths: They get accepted by the typechecker rather than flagged as non-existent. A correct workaround is: type Client = ReturnType<typeof createPathBasedClient<paths>>; |
To achieve this, we remove unnecessary type bounds.
This fixes #1840.
Changes
How to Review
Checklist
docs/updated (if necessary)pnpm run update:examplesrun (only applicable for openapi-typescript)