Allow "unknown" values to be boomified

[matthieusieben]

Support plan

• is this issue currently blocking your project? (yes/no): yes
• is this issue affecting a production system? (yes/no): yes

Context

• node version: *
• module version: 9.1.4
• environment (e.g. node, browser, native): *
• used with (e.g. hapi application, another framework, standalone, ...): Typescript >= 4.4
• any other relevant information: *

What problem are you trying to solve?

Since typescript 4.4, the type of the err in a catch block will be "unknown".

try {
  // stuff
} catch (err) {
  throw boomify(err) // <=== typescript error "Argument of type "unknown" cannot be assigned to type: Error"
}

There are two solutions, both of which result in a lot of added code

• First:

try {
  // stuff
} catch (err) {
  throw err instanceof Error ? boomify(err) : err
}

• Second:

// we have to import this everywhere when it is needed
const asError = (err: unknown): Error => err instanceof Error ? err : Object.assign(new Error(err?.message ?? 'Unknown error'), err)

try {
  // stuff
} catch (err) {
  throw boomify(asError(err))
}

Since boomify is already an error wrapper utility, having to do either of these is anoying.

Do you have a new or modified API suggestion to solve the problem?

Could we allow the first argument of boomify() to be of type unknown ?

If you don't want to change the current signature, we could also make this optional:

boomify(err as unknown, { allowUnknown: true })

[Nargonath]

I'm no TS wizard but I think using a flag would just make things more complicated. From my understanding of TS we should type it as err: Error | unknown if we hide the behaviour behind an option flag. Considering the nature of the unknown type, it seems counterproductive. We would still have to narrow down the type with some defensive code and handle every possible case for err when it's actually unknown. So we might as well just type it unknown. That way we don't need to do any conditional type checks to type the "on/off" behaviour based on the flag too.

Supporting unknown seems like a reasonable feature to add since that's the type boomify is most likely to encounter in the context it's usually used. If people pass an actual unknown type to boomify they most likely don't care how we're going to handle coercion to Error otherwise they'd just have to run their own logic before calling boomify. It makes sense baking some kind of default handling into the function IMO.

[Marsup]

Not really in favor of that, boom is not supposed to be a catch-all API, the constructor already accepts strings or errors which is I believe as far as it should go. You can just throw boomify(err as Error) if you feel confident enough in your code.

[kanongil]

I'm with @Marsup on this. Any non error will throw, so the current TS definition is correct: https://github.com/hapijs/boom/blob/12e025c3e873e94cd9e5bfa0e7f4c891c94ea2f6/lib/index.js#L111-L113

[Nargonath]

Alright, makes sense. I don't mind keeping it as is. @Marsup solution is simple enough.

[matthieusieben]

Typescript is there to ensure type safety. For example there is no point in doing this:

function inc(a: number) {
  if (typeof a !== 'number') throw new TypeError('a is expected to be a number')
  // ...
}

The whole point of using typescript is to ensure that, as long as you respect the type definitions (no as any etc.), you should be safe from running into an unexpected runtime error.

In the case of Boom, this is exactly what is happening: the boomify expects an Error as argument type AND checks for the type at runtime.

My point is it should do either, not both.

Now I agree that anything thrown or used as rejection should always be an error. So I don't mind doing this:

catch (err) {
  throw boomify(err as Error)
}

But that causes a lot of overhead (every catch block has to do this). And since boomify does perform a runtime check its implementation is de-facto type safe. For these reasons, I think that it should not be a problem to use unknown as type for its first argument in the .d.ts file.

catch (err) {
  throw boomify(err) // Note: a TypeError will be thrown instead if err is not an Error
}

[kanongil]

Error is still the correct type for the argument. If a non-Error is passed, another non-Boom Error will be thrown, which means that you did something wrong when calling the method. APIs should not throw for expected inputs.

We could however create a new method / option / breaking change that expects unknown instead, and return an appropriate Boom object on non-Errors instead of throwing. Eg. boomifyAny(err: unknown), or something along your option suggestion.

[kanongil]

Actually, a breaking change that always returns a Boom object is probably the best way forward. It is unfortunate that throw boomify(err) risks throwing a non-Boom error.

[kanongil]

Hapi already takes measures to ensure it doesn't pass non-Errors, which could be avoided.

It would also fix this url parsing logic, where a custom query parser can throw non-Errors, and cause a boomify() to throw instead of returning and assigning the result to this._urlError. I haven't tested it, but a non-Error thrown from a custom query parser probably crashes the server, as it is!

[matthieusieben]

Just to make things clear:

There are 2 things when it comes to typing a function using typescript. There is the "external" signature and then there is the typing of the arguments within the function's body. Usually they both are the same. But sometimes we need to distinguish the two (see Function Overloading).

In the current implementation, and if @hapi/boom was written 100% in typescript, you would have the following:

As you can see, err has type never inside the "if". So the current implementation contains a "unreachable code path", which can arguably be considered a bad practice too.

The proper TS implementation would be either:

export function boomify(err: unknown): Boom {
  if (!(err instanceof Error)) {
    throw new TypeError('Invalid err')
  }

  // Here "err" can safely be used as an Error
  return new Boom()
} 

or

export function boomify(err: Error): Boom {
  // no need to check for err's type at runtime
  return new Boom()
} 

What is currently implemented is more like this:

export function boomify(err: Error): Boom;
export function boomify(err: unknown): Boom {
  if (!(err instanceof Error)) {
    // This is included for extra safety when boom is used in environment not supporting type checking
    throw new TypeError('Invalid err')
  }

  // Here err can safely be used as an Error
  return new Boom()
}

As you can see the current implementation (3) restricts its public API in a way that is not needed for its own implementation to be type safe.

Now I really don't think that boomify (or is new version) should never throw. It is actually sound & a good practice to throw when you encounter a piece of code that throws something else than an error.

This is why I think that we should have the ability pass any argument to boomify and get a "system error" if that fails. What I mean is that only the typing should be changed, not the implementation.

And since that would be a breaking change, we need either a new option to boomify or a new function altogether (probably best).

See this example (playground):

Adding a new option would be kind of silly as boomify(err, { allowUnknwon: true }) is actually worse than writing boomify(err as Error). So a new function (like boomifyAny) would probably be the way to go.

Additional reflections:

• It is sometimes good to crash the server when a piece of code throws something else than an error. (what if the developper mistakenly types throw result instead of return result in your URL parsing example)
• Usually when we use boomify() we intent to get a 500, whatever the value of err is.

[matthieusieben]

Maybe something like:

declare function boomifyAny(err: unknwon, { allowNonError = true }: { allowNonError: boolean });

if allowNonError is true then a non Error would result in a badImplementation if allowNonError is false then a non Error would result in a TypeError

[kanongil]

As I see it, the current implementation and type signature would also be valid for TS, except the check would be done using an assertion.

It is not unheard of to assert that the passed parameters are what you expect, especially for public APIs and where the passed err could have been thrown from any type of code of unknown origin, maybe even a callback method provided by a user.

Now I really don't think that boomify (or is new version) should never throw. It is actually sound & a good practice to throw when you encounter a piece of code that throws something else than an error.

That really depends on the context. Eg. for hapi handlers, we always want a Boom object, regardless of the type of the thrown error. In fact both of my examples are better served by not throwing.

[matthieusieben]

I made a few tests:

https://github.com/matthieusieben/boom/commit/31b80ef5464a5c45bd58eeac2efa021c07c71c77 https://github.com/matthieusieben/boom/commit/dcb1e26f01e80776dbbb802bc2928c6c3abc7f2f https://github.com/matthieusieben/boom/commit/af02cea4da1e93522bad3136cf015a00bf006d34 https://github.com/matthieusieben/boom/commit/6cc10af8e5645725a9db537b38eebc4b8bab7c88 https://github.com/matthieusieben/boom/commit/9df4be2ece6e3be865f2f288521d701d1abf9cd6

And a PR https://github.com/hapijs/boom/pull/293

Let me know what you think

[devinivy]

Bumping this and the PR #293 for any further discussion/review/thoughts.

[Marsup]

Still not sure about this. Typescript is a pita in this area, there are whole articles about this. We could relax the api, it would be caught by the assertion anyway, but it seems like a footgun at runtime. If we remain strict about it, the error type would be double-checked, so it's not ideal either.

There's no good decision here, but usually errors are errors, and we have the safety net, so maybe just relax the signature, it doesn't seem any different from the usual assumption most of us do in standard js, nobody checks the type of thrown errors :man_shrugging:

(sorry for the swinging opinion)

[matthieusieben]

As I see it, the options are:

0. do nothing an keep things as they are
1. only change the typing of boomify to accept any/unknown so that TS does not complain when boomifying in a catch block. TypeError would still be thrown
2. change the implementation of boomify to accept any/unknown and so that is never throws (always return a Boom instance, regerdless of the argument) (as suggested by kanongil)
3. Avoid breaking changes by creating a new signature (boomifyAny or boomify(any, { allowNonError: true }) or whathever) (as seen in #293)
4. Change the default behavior to turn anything into an Error (never throw TypeError) (as suggested by kanongil) but keep current behavior as an opt-in through a new signature (boomifyError or boomify(any, { expectError: true }) or boomify(any, { expectType: Error }) or whatever) to be able to handle non-errors more strictly in specific cases.

There are two kinds of breaking changes here:

• Those that only apply to developers
• Those that have repercussions at runtime

I believe that it is safer to avoid making a breaking change that impacts runtime (= "soft" breaking change). But I think that having the ability to safely turn anything into an error could really be an added benefit of Boom.

Edit: added option 4)

[matthieusieben]

Let's agree on the option to follow before we agree on the potential change in signature 😀

[kanongil]

Thanks for the overview. As you probably guess I favour option 2. Option 4 seems intriguing, but I would prefer not to go that route, unless someone can demonstrate a case where the option makes sense. I much prefer to keep the API simple (also the main reason I don't like option 3).

I believe that it is safer to avoid making a breaking change that impacts runtime (= "soft" breaking change).

In this specific case, I'm disinclined to agree since the error throwing is already causing unintended behaviour, and never throwing is likely to fix subtle bugs, see https://github.com/hapijs/boom/issues/291#issuecomment-997899355 for details. FYI, just verified that throwing a non-Error from a custom query parser does crash the server.

[devinivy]

In the case of no. 2, what error would be returned: would it be a badImplementation() with the passed non-error set as data? Would it respect the statusCode option?

[matthieusieben]

I'm gonna let you guys decide on that ;-)

FWIW, here is what I would do:

if (!(err instanceof Error)) {
  return badImplementation('Trying to turn a non-error into an error', err)
}

[kanongil]

I would probably do it like new Boom.Boom().

Having another look at it, I'm conflicted about going for 2, since boomify is clearly documented to only work on errors and to work on existing objects, while new Boom.Boom() is already designed for when you don't know what you have.

I think I'm mostly in favour of option 0 now. And possibly update the Boom constructor() to accept any as the first arguments, since it accepts any inputs value and essentially stringifies any non-falsy non-Error value.

[matthieusieben]

Well new Boom is indeed essentially the same as the boomifyAny() I suggested in my PR.

Can I suggest another options then: 5. Allow constructor to accept any as first argument & improve the message when the constructor is called with a non-string - non-Error value to avoid having Error: [object Object]

[matthieusieben]

Maybe also default data to message when message is not a string ? Something like:

        const useMessage = typeof message === 'string'
        const { statusCode = 500, data = useMessage ? null : message ?? null, ctor = exports.Boom } = options;
        const error = new Error(useMessage ? message : undefined);

[Marsup]

After taking some time to think about it, maybe option 3 is not that bad. A helper inside your app is not that hard to create and import, but if every typescript developer out there ends up sharing the same snippet, might as well integrate it into boom. Just have to come up with a name that makes sense.

[laurilarjo]

I just started to use Boom in typescript, and came here to see why boomify doesn't support the unknown errors, which is the most common use case.

I'd go for option 1 or 2 for simplicity since all the guides already teach you to use boomify(). Even though it changes the current function's signature, it doesn't cause any issues for existing projects when they upgrade, since it just makes the signature looser. Adding something like boomifyAny or boomifyError would only cause confusion to people who're starting with Boom.