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:

boom/lib/index.js

Lines 111 to 113 in 12e025c

	exports.boomify = function (err, options) {
	Hoek.assert(err instanceof Error, 'Cannot wrap non-Error object');

[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:

matthieusieben@31b80ef
matthieusieben@dcb1e26
matthieusieben@af02cea
matthieusieben@6cc10af
matthieusieben@9df4be2

And a PR #293

Let me know what you think