-
-
Notifications
You must be signed in to change notification settings - Fork 544
fix(openapi-fetch): Return union of possible responses (data & error) #1937
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.
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
Changes from all commits
f8df0e1
4b4fb7a
6e354a8
ed3edeb
0c915ba
55da875
b9501b7
ef577d1
62f0329
cb027ec
d307e52
080673d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
--- | ||
"openapi-typescript-helpers": patch | ||
"openapi-fetch": patch | ||
--- | ||
|
||
client data & error now return a union of possible types |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
import { assertType, describe, expect, test } from "vitest"; | ||
import { createObservedClient } from "../helpers.js"; | ||
import type { components, paths } from "./schemas/never-response.js"; | ||
|
||
describe("GET", () => { | ||
test("sends correct method", async () => { | ||
let method = ""; | ||
const client = createObservedClient<paths>({}, async (req) => { | ||
method = req.method; | ||
return Response.json({}); | ||
}); | ||
await client.GET("/posts"); | ||
expect(method).toBe("GET"); | ||
}); | ||
|
||
test("sends correct options, returns success", async () => { | ||
const mockData = { | ||
id: 123, | ||
title: "My Post", | ||
}; | ||
|
||
let actualPathname = ""; | ||
const client = createObservedClient<paths>({}, async (req) => { | ||
actualPathname = new URL(req.url).pathname; | ||
return Response.json(mockData); | ||
}); | ||
|
||
const { data, error, response } = await client.GET("/posts/{id}", { | ||
params: { path: { id: 123 } }, | ||
}); | ||
|
||
assertType<typeof mockData | undefined>(data); | ||
|
||
// assert correct URL was called | ||
expect(actualPathname).toBe("/posts/123"); | ||
|
||
// assert correct data was returned | ||
expect(data).toEqual(mockData); | ||
expect(response.status).toBe(200); | ||
|
||
// assert error is empty | ||
expect(error).toBeUndefined(); | ||
}); | ||
|
||
test("sends correct options, returns undefined on 204", async () => { | ||
let actualPathname = ""; | ||
const client = createObservedClient<paths>({}, async (req) => { | ||
actualPathname = new URL(req.url).pathname; | ||
return new Response(null, { status: 204 }); | ||
}); | ||
|
||
const { data, error, response } = await client.GET("/posts/{id}", { | ||
params: { path: { id: 123 } }, | ||
}); | ||
|
||
assertType<components["schemas"]["Post"] | undefined>(data); | ||
|
||
// assert correct URL was called | ||
expect(actualPathname).toBe("/posts/123"); | ||
|
||
// assert 204 to be transformed to empty object | ||
expect(data).toEqual({}); | ||
Comment on lines
+61
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Side-note: I do not fully agree with this behaviour at https://github.com/openapi-ts/openapi-typescript/blob/main/packages/openapi-fetch/src/index.js#L155.
This PR ensures that a documented 204 response ( Would love to hear some feedback and if it could be considered to remove this 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes we’re open to revisiting that behavior! Agree that at this point, that does more harm than good. That was an early behavior that just never received much questioning until now. I’d be open to either of the following:
Either way it would constitute a breaking change, but IMO a welcome one. No strong preference on #1 or #2, but #1 would be easier to release faster There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My preference would also be to keep this PR as-is aswell. I will make a new PR to fix the After that I will have a look at what @zkulbeda suggested for exposing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That works! Also noticed there’s a tiny conflict with the openapi-typescript-helpers; can merge once that’s resolved. Also feel free to make an adjustment there if it causes a regression; that recent change was just an arbitrary QoL improvement and didn’t fix any type issues like your PR does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! Fixed the feedback you gave so everything should be good now 😄 |
||
expect(response.status).toBe(204); | ||
|
||
// assert error is empty | ||
expect(error).toBeUndefined(); | ||
}); | ||
|
||
test("sends correct options, returns error", async () => { | ||
const mockError = { code: 404, message: "Post not found" }; | ||
|
||
let method = ""; | ||
let actualPathname = ""; | ||
const client = createObservedClient<paths>({}, async (req) => { | ||
method = req.method; | ||
actualPathname = new URL(req.url).pathname; | ||
return Response.json(mockError, { status: 404 }); | ||
}); | ||
|
||
const { data, error, response } = await client.GET("/posts/{id}", { | ||
params: { path: { id: 123 } }, | ||
}); | ||
|
||
assertType<typeof mockError | undefined>(error); | ||
|
||
// assert correct URL was called | ||
expect(actualPathname).toBe("/posts/123"); | ||
|
||
// assert correct method was called | ||
expect(method).toBe("GET"); | ||
|
||
// assert correct error was returned | ||
expect(error).toEqual(mockError); | ||
expect(response.status).toBe(404); | ||
|
||
// assert data is empty | ||
expect(data).toBeUndefined(); | ||
}); | ||
|
||
test("handles array-type responses", async () => { | ||
const client = createObservedClient<paths>({}, async () => Response.json([])); | ||
|
||
const { data } = await client.GET("/posts", { params: {} }); | ||
if (!data) { | ||
throw new Error("data empty"); | ||
} | ||
|
||
// assert array type (and only array type) was inferred | ||
expect(data.length).toBe(0); | ||
}); | ||
|
||
test("handles empty-array-type 204 response", async () => { | ||
let method = ""; | ||
let actualPathname = ""; | ||
const client = createObservedClient<paths>({}, async (req) => { | ||
method = req.method; | ||
actualPathname = new URL(req.url).pathname; | ||
return new Response(null, { status: 204 }); | ||
}); | ||
|
||
const { data } = await client.GET("/posts", { params: {} }); | ||
|
||
assertType<components["schemas"]["Post"][] | unknown[] | undefined>(data); | ||
|
||
// assert correct URL was called | ||
expect(actualPathname).toBe("/posts"); | ||
|
||
// assert correct method was called | ||
expect(method).toBe("GET"); | ||
|
||
// assert 204 to be transformed to empty object | ||
expect(data).toEqual({}); | ||
}); | ||
|
||
test("gracefully handles invalid JSON for errors", async () => { | ||
const client = createObservedClient<paths>({}, async () => new Response("Unauthorized", { status: 401 })); | ||
|
||
const { data, error } = await client.GET("/posts"); | ||
|
||
expect(data).toBeUndefined(); | ||
expect(error).toBe("Unauthorized"); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,142 @@ | ||
/** | ||
* This file was auto-generated by openapi-typescript. | ||
* Do not make direct changes to the file. | ||
*/ | ||
|
||
export interface paths { | ||
"/posts": { | ||
parameters: { | ||
query?: never; | ||
header?: never; | ||
path?: never; | ||
cookie?: never; | ||
}; | ||
get: { | ||
parameters: { | ||
query?: never; | ||
header?: never; | ||
path?: never; | ||
cookie?: never; | ||
}; | ||
requestBody?: never; | ||
responses: { | ||
/** @description OK */ | ||
200: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content: { | ||
"application/json": components["schemas"]["Post"][]; | ||
}; | ||
}; | ||
/** @description No posts found, but it's OK */ | ||
204: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content: { | ||
"application/json": unknown[]; | ||
}; | ||
}; | ||
/** @description Unexpected error */ | ||
default: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content: { | ||
"application/json": components["schemas"]["Error"]; | ||
}; | ||
}; | ||
}; | ||
}; | ||
put?: never; | ||
post?: never; | ||
delete?: never; | ||
options?: never; | ||
head?: never; | ||
patch?: never; | ||
trace?: never; | ||
}; | ||
"/posts/{id}": { | ||
parameters: { | ||
query?: never; | ||
header?: never; | ||
path: { | ||
id: number; | ||
}; | ||
cookie?: never; | ||
}; | ||
get: { | ||
parameters: { | ||
query?: never; | ||
header?: never; | ||
path: { | ||
id: number; | ||
}; | ||
cookie?: never; | ||
}; | ||
requestBody?: never; | ||
responses: { | ||
/** @description OK */ | ||
200: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content: { | ||
"application/json": components["schemas"]["Post"]; | ||
}; | ||
}; | ||
/** @description No post found, but it's OK */ | ||
204: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content?: never; | ||
}; | ||
/** @description A weird error happened */ | ||
500: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content?: never; | ||
}; | ||
/** @description Unexpected error */ | ||
default: { | ||
headers: { | ||
[name: string]: unknown; | ||
}; | ||
content: { | ||
"application/json": components["schemas"]["Error"]; | ||
}; | ||
}; | ||
}; | ||
}; | ||
put?: never; | ||
post?: never; | ||
delete?: never; | ||
options?: never; | ||
head?: never; | ||
patch?: never; | ||
trace?: never; | ||
}; | ||
} | ||
export type webhooks = Record<string, never>; | ||
export interface components { | ||
schemas: { | ||
Error: { | ||
code: number; | ||
message: string; | ||
}; | ||
Post: { | ||
id: number; | ||
title: string; | ||
}; | ||
}; | ||
responses: never; | ||
parameters: never; | ||
requestBodies: never; | ||
headers: never; | ||
pathItems: never; | ||
} | ||
export type $defs = Record<string, never>; | ||
export type operations = Record<string, never>; |
Uh oh!
There was an error while loading. Please reload this page.