-
-
Notifications
You must be signed in to change notification settings - Fork 655
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
perf(hono-base): don't import HTTPException
#2885
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2885 +/- ##
=======================================
Coverage 87.88% 87.88%
=======================================
Files 139 139
Lines 14208 14215 +7
Branches 2314 2338 +24
=======================================
+ Hits 12486 12493 +7
Misses 1722 1722 ☔ View full report in Codecov by Sentry. |
Hey @usualoma. I want to know your thoughts on this. |
@yusukebe Alternatively, how about the following. It simplifies the definition and opens up the possibility for users themselves to use "errors implementing getResponse()", other than hono’s diff --git a/src/hono-base.ts b/src/hono-base.ts
index f3e9ad03..858d65db 100644
--- a/src/hono-base.ts
+++ b/src/hono-base.ts
@@ -7,7 +7,6 @@
import { compose } from './compose'
import { Context } from './context'
import type { ExecutionContext } from './context'
-import { HTTPException } from './http-exception'
import { HonoRequest } from './request'
import type { Router } from './router'
import { METHODS, METHOD_NAME_ALL, METHOD_NAME_ALL_LOWERCASE } from './router'
@@ -21,6 +20,7 @@ import type {
MergeSchemaPath,
MiddlewareHandler,
MiddlewareHandlerInterface,
+ HTTPResponseError,
Next,
NotFoundHandler,
OnHandlerInterface,
@@ -38,8 +38,8 @@ const notFoundHandler = (c: Context) => {
return c.text('404 Not Found', 404)
}
-const errorHandler = (err: Error, c: Context) => {
- if (err instanceof HTTPException) {
+const errorHandler = (err: Error | HTTPResponseError, c: Context) => {
+ if ('getResponse' in err) {
return err.getResponse()
}
console.error(err)
diff --git a/src/types.ts b/src/types.ts
index 2178e5c1..87691b01 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -91,8 +91,12 @@ export type H<
> = Handler<E, P, I, R> | MiddlewareHandler<E, P, I>
export type NotFoundHandler<E extends Env = any> = (c: Context<E>) => Response | Promise<Response>
+
+export interface HTTPResponseError extends Error {
+ getResponse: () => Response
+}
export type ErrorHandler<E extends Env = any> = (
- err: Error,
+ err: Error | HTTPResponseError,
c: Context<E>
) => Response | Promise<Response> |
@usualoma Thank you for the suggestion! I also thought it was okay to check |
Can you create the new PR with your patch? |
This change is intended to make error handling more flexible and reduce bundle size. This idea is originated from honojs#2885 Co-authored-by: Yusuke Wada <[email protected]>
This change is intended to make error handling more flexible and reduce bundle size. This idea is originated from honojs#2885 Co-authored-by: Yusuke Wada <[email protected]>
…#2898) This change is intended to make error handling more flexible and reduce bundle size. This idea is originated from #2885 Co-authored-by: Yusuke Wada <[email protected]>
This is not necessary now since #2898 has been merged. |
This is like a
refactor
but is made aperf
since reducing the bundle size for the performance.With this PR, the
hono-base.ts
does not need to import theHTTPException
class directly. So, the bundle size for the app that does not useHTTPException
will be small. To enable it, it does not useinstanceOf
; it will detect by checking the property named a symbol ofIS_HTTP_EXCEPTION.
Result:
It was reduced by 260 bytes. It seems small, but reducing size is important for edge environments such as Cloudflare Workers.
The author should do the following, if applicable
bun run format:fix && bun run lint:fix
to format the code