-
Notifications
You must be signed in to change notification settings - Fork 7
fix: Improve handling of HEAD requests when using hono plugin #103
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
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,23 @@ | ||
| /** @type {import('ts-jest/dist/types').InitialOptionsTsJest} */ | ||
| module.exports = { | ||
| transform: { | ||
| "^.+\\.tsx?$": [ | ||
| "ts-jest", | ||
| { | ||
| diagnostics: false, | ||
| isolatedModules: true, | ||
| }, | ||
| ], | ||
| }, | ||
| testEnvironment: "node", | ||
| modulePathIgnorePatterns: ["<rootDir>/node_modules/", "/dist/"], | ||
| testPathIgnorePatterns: ["<rootDir>/node_modules/", "/dist/"], | ||
| testMatch: ["**/__tests__/**/*.test.[jt]s?(x)"], | ||
| setupFilesAfterEnv: [], | ||
| watchPathIgnorePatterns: ["<rootDir>/node_modules/"], | ||
| watchPlugins: [ | ||
| "jest-watch-typeahead/filename", | ||
| "jest-watch-typeahead/testname", | ||
| ], | ||
| verbose: false, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,6 @@ | ||
| import { Hono } from "hono"; | ||
| import { Stl, UnauthorizedError, z } from "stainless"; | ||
| import { describe, expect, test } from "vitest"; | ||
| import { stlApi } from "./honoPlugin"; | ||
| import { stlApi } from "../honoPlugin"; | ||
|
|
||
| const stl = new Stl({ plugins: {} }); | ||
|
|
||
|
|
@@ -63,9 +62,7 @@ describe("basic routing", () => { | |
| test("list posts", async () => { | ||
| const response = await app.request("/api/posts"); | ||
| expect(response).toHaveProperty("status", 200); | ||
| expect(await response.json()).toMatchInlineSnapshot(` | ||
| [] | ||
| `); | ||
| expect(await response.json()).toMatchInlineSnapshot("[]"); | ||
| }); | ||
|
|
||
| test("retrieve posts", async () => { | ||
|
|
@@ -85,11 +82,19 @@ describe("basic routing", () => { | |
| expect(response).toHaveProperty("status", 405); | ||
| expect(await response.json()).toMatchInlineSnapshot(` | ||
| { | ||
| "message": "No handler for PUT; only GET, POST.", | ||
| "message": "No handler for PUT; only GET, HEAD, POST.", | ||
| } | ||
| `); | ||
| }); | ||
|
|
||
| test("head posts", async () => { | ||
|
Contributor
Author
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. This test fails in main and passes with the fix. |
||
| const response = await app.request("/api/posts/5", { | ||
| method: "HEAD", | ||
| }); | ||
| expect(response).toHaveProperty("status", 200); | ||
| expect(await response.text()).toBe(""); | ||
| }); | ||
|
|
||
| test("update posts", async () => { | ||
| const response = await app.request("/api/posts/5", { | ||
| method: "POST", | ||
|
|
@@ -159,7 +164,7 @@ describe("basic routing", () => { | |
| "received": "undefined", | ||
| }, | ||
| ], | ||
| "message": "Validation error: Required at "<body>.content"", | ||
| "message": "Required at "<body>.content"", | ||
| } | ||
| `); | ||
| }); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,11 @@ export function makeRouteMatcher(endpoints: AnyEndpoint[]) { | |
| for (const endpoint of endpoints) { | ||
| const [method, path] = endpointToHono(endpoint.endpoint); | ||
| routeMatcher.add(method, path, endpoint); | ||
| if (method === "GET") { | ||
|
Contributor
Author
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. The actual fix. |
||
| // Hono route matching is method-specific, so also add a | ||
| // HEAD route for GET endpoints | ||
| routeMatcher.add("HEAD", path, endpoint); | ||
| } | ||
| } | ||
|
|
||
| return routeMatcher; | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: oops this was my bad. if we're going to use jest we should remove
vitestin devdepsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!