Let app.onError handle non-Error instance

[universse]

Currently, if any route throws a non-Error instance, app.onError won't be invoked. Users need to handle those separately for each route. It would be nice if that can be handled by app.onError.

[yusukebe]

Hi @universse !

Thank you for your proposal. It seems to be good. One question.

a non-Error instance

What kind of "non-Error instance" do you mean? I want to know your use case.

[universse]

I'm making request to a 3rd party API using a custom fetch wrapper that throws any non-success response (!response.ok). The API returns a json response with status 403, which is thrown by the fetch wrapper and get caught by the route handler, like this

I end up with this page when making request via the browser.

[ctsstc]

I ran into this today while letting AI write a test scenario. It had a dependency throw a non-error like so: throw 'String error'; // Non-Error object I was confused why Hono wasn't handling the test scenario, and why the test suite was breaking, the throw made it up into the test suite and prevented some cleanup code from running which was furthermore confusing throwing additional errors and chaos.

This seems like a confusing footgun essentially, because the onError handler doesn't get triggered. Any third party/bad code could throw whatever and Hono could then miss it. Our aim is to deliver a reliable consistent error message back to clients no matter what the scenario is.

Related Issues

Many of these ran into the footgun it seems, and could have been avoided if onError caught non-error types.

• https://github.com/honojs/hono/issues/1267 (duplicate with deeper discussion)
• https://github.com/honojs/hono/issues/2462 Marked as completed w/o discussion or any artifact
• #4241
• https://github.com/honojs/hono/issues/2533
• https://github.com/honojs/hono/issues/3252
• https://github.com/honojs/hono/issues/3810

Thoughts

Maybe the answer is to throw this into the documentation?

I lean towards catching them all though.

If it is a matter of type safety/inference, unknown errors should still be caught and brought to attention.

• Either a new type could be added to denote this scenario like an UnknownError, or additional context could be added potentially.
• Maybe there needs to be another callback handler added to at least move this forward like an .onUnknownError.

It's concerning that a 3rd party library or ourselves could throw something that may not be caught within the onError handler.

It seems dirty to have us wrap our entire router/app in a try/catch block. I guess it likely needs to happen somewhere though?

[yusukebe]

Hi @ctsstc

Thank you for the comment and your thoughts.

It seems dirty to have us wrap our entire router/app in a try/catch block.

I also think it's not good to force users to wrap their entire app in try/catch. Then, onUnknownError you suggest makes sense.

Hi @usualoma I want to know your opinion.

[yusukebe]

The strength of onUnknownError is that it does not introduce a breaking change.

[ctsstc]

I think adding something like onUnknownError would be a good transition that wouldn't introduce a breaking change. Then later on during a major release you could refactor a new UnknownError type into the onError handler. I would expect that some folks would utilize the same handler for both anyways before a major release occurred.

Example

function errorHandler(err: Error | HTTPResponseError | UnknownError, c) {}

app.onError(errorHandler).onUnknownError(errorhandler);

Then after a major release the migration is rather simple.

[ngraef]

What kind of "non-Error instance" do you mean? I want to know your use case.

I ran into this with a library that throws Error-like objects that have the same shape but don't use the Error constructor. Testing this middleware right now, and it seems to fix the problem in my case:

// Handle Error-like exceptions and other non-Error Promise rejections so they're
// handled by `app.onError`, which is only triggered for instances of Error.
app.use(async (ctx, next) => {
    try {
        await next();
    } catch (err) {
        // If it's already an Error, rethrow it
        if (err instanceof Error) {
            throw err;
        }

        // Turn Error-like objects into Errors
        if (typeof err === 'object' && err !== null) {
            const e = new Error();
            if ('name' in err && typeof err.name === 'string') {
                e.name = err.name;
            }
            if ('message' in err && typeof err.message === 'string') {
                e.message = err.message;
            }
            if ('stack' in err && typeof err.stack === 'string') {
                e.stack = err.stack;
            }
            if ('cause' in err) {
                e.cause = err.cause;
            }
            throw e;
        }

        // Use strings as the error message
        if (typeof err === 'string') {
            throw new Error(err);
        }

        // Wrap other things in an Error
        throw new Error('Unknown error', { cause: err });
    }
});

However, it needs to be registered after other middlewares (like logger) that have logic after await next(), otherwise their functionality will break in the non-Error case.

[usualoma]

Hi @yusukebe, sorry I missed the discussion. I'm catching up now.

[usualoma]

I think it's best to consider the final (major release?) changes first.

As https://github.com/honojs/hono/issues/667#issuecomment-3234181742 commented, if it's going to end up like this anyway, it's better to do it that way from the start. (It depends on what UnknownError refers to; if it's a wrapped Error instance, it might be the same as below.)

type ErrorHandler<E extends Env = any> = (
  err: Error | HTTPResponseError | UnknownError,
  c: Context<E>
) => Response | Promise<Response>

Or, as https://github.com/honojs/hono/issues/667#issuecomment-3259640174 commented, if we're going to wrap it unless it's an Error instance, I still think it's better to make the change in that direction from the beginning.

I'm currently somewhat opposed to introducing onUnknownError. While I think onUnknownError is an approach that can solve problems without compatibility concerns, APIs are difficult to remove once introduced, and having both onError and onUnknownError seems a bit confusing.

[yusukebe]

@usualoma

Thank you for the reply!

I also think onUnknownError is not good when I implemented the draft https://github.com/honojs/hono/pull/4411. It will be complex and verbose. It's just a perspective for implementation. And I also agree with you on other things. I think the best way is to introduce an allowance for unknown to the ErrorHandler in the next major release.

By the way. Regarding the major release, I think we can make it not so far. We don't have to include flashy things in the major version.

[usualoma]

Ultimately, it depends on the final specification, but existing apps likely operate under the principle of “catching exceptions beforehand if they're not thrown as an Error instance.” Therefore, even if those (originally ! (err instanceof Error)) are passed to ErrorHandler, existing apps should be largely unaffected. I feel like a minor version update might suffice.

Of course, I think it's fine to bump the major version as well.

[yusukebe]

Ah, they may face only type issues.

[usualoma]

I believe there are several ways to do this, but I think it's fine to proceed as follows.

diff --git i/src/compose.ts w/src/compose.ts
index b1d4508f..42379ef1 100644
--- i/src/compose.ts
+++ w/src/compose.ts
@@ -50,9 +50,13 @@ export const compose = <E extends Env = Env>(
         try {
           res = await handler(context, () => dispatch(i + 1))
         } catch (err) {
-          if (err instanceof Error && onError) {
-            context.error = err
-            res = await onError(err, context)
+          if (onError) {
+            const error =
+              err instanceof Error
+                ? err
+                : new Error(typeof err === 'string' ? err : undefined, { cause: err })
+            context.error = error
+            res = await onError(error, context)
             isError = true
           } else {
             throw err

The reasons are as follows:

• Minimal impact on existing users of onError
• Continued avoidance of unnecessary type checking with onError (specifying unknown or any would require initial type checking)
  • Since Hono's middleware and handlers generally expect throwing Error instances, only exceptional applications that throw an Object or an string need to retrieve them from error.cause
• Minimal code increase

[usualoma]

Ah, but fundamentally, in cases where non-Error objects are thrown, I believe the best practice is to handle them yourself by catching them, rather than relying on onError.

Given that aspect, I think it's best to keep changes minimal and simply ensure the behavior remains “does not cause abnormal termination even if non-Error instances are thrown.”

[yusukebe]

It's not a bad idea to put a thrown non-error value to Error like the comment: https://github.com/honojs/hono/issues/667#issuecomment-3301249718

But, I do not feel good that it will all be an Error object. So, how about creating an UnknownErrorobject that is a subclass of Error like this?

// unknown-error.ts
export class UnknownError extends Error {
  constructor(err: unknown) {
    if (typeof err === 'string') {
      super(err)
    } else {
      super(undefined)
      this.cause = err
    }
  }
}

If so, we can go with minimal chnages:

diff --git a/src/compose.ts b/src/compose.ts
index b1d4508f..e7013f6c 100644
--- a/src/compose.ts
+++ b/src/compose.ts
@@ -1,5 +1,6 @@
 import type { Context } from './context'
 import type { Env, ErrorHandler, Next, NotFoundHandler } from './types'
+import { UnknownError } from './unknown-error'

 /**
  * Compose middleware functions into a single function based on `koa-compose` package.
@@ -50,9 +51,9 @@ export const compose = <E extends Env = Env>(
         try {
           res = await handler(context, () => dispatch(i + 1))
         } catch (err) {
-          if (err instanceof Error && onError) {
-            context.error = err
-            res = await onError(err, context)
+          if (onError) {
+            context.error = err instanceof Error ? err : new UnknownError(err)
+            res = await onError(context.error, context)
             isError = true
           } else {
             throw err
diff --git a/src/types.ts b/src/types.ts
index e31b8a2a..66222881 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -7,6 +7,7 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
 import type { Context } from './context'
 import type { HonoBase } from './hono-base'
+import type { UnknownError } from './unknown-error'
 import type { CustomHeader, RequestHeader } from './utils/headers'
 import type { StatusCode } from './utils/http-status'
 import type {
@@ -96,7 +97,7 @@ export interface HTTPResponseError extends Error {
   getResponse: () => Response
 }
 export type ErrorHandler<E extends Env = any> = (
-  err: Error | HTTPResponseError,
+  err: Error | HTTPResponseError | UnknownError,
   c: Context<E>
 ) => Response | Promise<Response>

But, I wonder if it's best or not to treat non-error things as a subclass of Error.

[yusukebe]

Hi @usualoma

I'd like to know the explanation of https://github.com/honojs/hono/issues/667#issuecomment-3301291099

Ah, but fundamentally, in cases where non-Error objects are thrown, I believe the best practice is to handle them yourself by catching them, rather than relying on onError.

What is an example of handling non-error objects without relying on onError?

Given that aspect, I think it's best to keep changes minimal and simply ensure the behavior remains “does not cause abnormal termination even if non-Error instances are thrown.”

To avoid abnormal termination, how can we do? Should we implement it hono self?

[usualoma]

Hi @yusukebe Sorry for not explaining clearly enough. Here's my opinion.

What is an example of handling non-error objects without relying on onError?

Suppose there was an app like the following.

const customFetch = async () => {
  if (Math.random() > 0.5) {
    throw {
      status: 400,
      body: 'error',
    }
  }
  return new Response('text');
}

const app = new Hono()

app.get("/", async () => {
  return customFetch();
});

export default app

I think it would be better to handle it as follows.

const customFetch = async () => {
  if (Math.random() > 0.5) {
    throw {
      status: 400,
      body: 'error',
    }
  }
  return new Response('text')
}

const customFetchWrapper = async () => {
  return customFetch().catch((e) => {
    return new Response(e.body, {
      status: e.status,
      headers: {
        'Content-Type': 'text/plain',
      }
    })
  })
}

const app = new Hono()

app.get('/', async () => {
  return customFetchWrapper()
})

export default app

Or perhaps like this

const customFetch = async () => {
  if (Math.random() > 0.5) {
    throw {
      status: 400,
      body: 'error',
    }
  }
  return new Response('text')
}

const customFetchWrapper = async () => {
  return customFetch().catch((e) => {
    const err = new Error(e.body)
    err.getResponse = () => {
      return new Response(e.body, {
        status: e.status,
        headers: {
          'Content-Type': 'text/plain',
        }
      })
    }
    throw err
  })
}

const app = new Hono()

app.get('/', async () => {
  return customFetchWrapper()
})

export default app

When something that is not an instance of Error is thrown, I believe it is better to handle it within the handler itself rather than simply throwing it from the handler to be handled by onError. This is because I believe the application possesses sufficient knowledge about what “non-Error instances” are, and can determine whether they should be treated as errors or caught and returned as normal responses. That said, it's merely “ideally preferable.” I don't think using onError in this situation is necessarily bad.

To avoid abnormal termination, how can we do? Should we implement it hono self?

While “the ideal approach is to catch exceptions within the application,” the purpose of the above patch is to address the problem that “even so, unexpected exceptions can occur, and in such cases, we don't want the application to terminate abnormally.”

[usualoma]

UnknownError

Defining a class that inherits from Error seems fine, but I find proper naming tricky.

UnknownError is indeed “unknown” in the sense that it represents ‘something’ that isn't an Error object, but it doesn't convey an “extended property” like HTTPResponseError does, so it looks a bit out of place when listed. (Error | HTTPResponseError | UnknownError)

Since wrapping the object caught in try/catch into cause is a common operation, I decided a simple Error would suffice, leading to the previous patch.

[yusukebe]

@usualoma

I also think UnknownError is not the best.

But making the class extend Error is useful because the user can determine if it's a pure error or not by using instanceOf. Or do they not need to determine it?

[usualoma]

Hi @yusukebe

Yes, I agree that defining a class as you suggest has the advantage of being identifiable via instanceof.

However, (in my opinion) this case is, as the name UnknownError suggests, an error where “Hono doesn't know what it is,” so I think it's fine to just use Error. If a user wants to confirm “it's an object I threw,” they can check with err.cause instanceof MyObject.

That said, there's no harm in creating a new error class if we have a good name for it.

[yusukebe]

Hi @usualoma

I'm still thinking about this issue.

However, (in my opinion) this case is, as the name UnknownError suggests, an error where “Hono doesn't know what it is,” so I think it's fine to just use Error. If a user wants to confirm “it's an object I threw,” they can check with err.cause instanceof MyObject.

Just using Error seems to be good, but I found it is impossible to determine whether it is throwing Error or throwing a non-Error object in some cases.

This means if using the code you provided, imagine the following handlers:

const nonErrorObject = { body: 'Something went wrong' }

const handlerA = () => {
  throw nonErrorObject
}

const handlerB = () => {
  throw new Error(undefined, { cause: nonErrorObject })
}

So, the e in app.onError will be the Error as the same structure and determining it from handlerA or handlerB is impossible, right?

app.onError((e) => {
  e // is new Error(undefined, { cause: nonErrorObject })??
})

I know it's a rare case having like handlerB, but what do you think of this (sorry if I misunderstood!)?

[yusukebe]

Opps. Accidentally, I closed. Reopened.

[usualoma]

Hi @yusukebe, thank you for your response!

My opinion on that point is: “There's no need to distinguish between handlerA and handlerB. If distinguishing becomes necessary, the user should generate the error themselves at that time.”

However, I can understand your perspective. If we decide to introduce a new error class, I support that choice.