Add support for native `cause`

matthieusieben commented 1 year ago

Support plan

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

Context

node version: n.a.
module version: n.a.
environment (e.g. node, browser, native): n.a.
used with (e.g. hapi application, another framework, standalone, ...): n.a.
any other relevant information: n.a.

What problem are you trying to solve?

Errors now have a cause property described here: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

We should be able to easily set the cause when creating a Boom error.

Currently, the following must be done:

try {
  // Stuff
} catch (err) {
  throw Object.assign(badImplementation('Stuff should not throw an error'), { cause: err })
}

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

It is my understanding that currently, in the Hapi ecosystem, the data property of Boom errors is sometimes used to contain the original error, so anything proposed here will likely largely impact the Hapi ecosystem.

in order not to break the current api too much, we could just add a new option on error creators:

export function badImplementation<Data>(message?: string, data?: Data, errorOptions?: { cause?: unknown }): Boom<Data>;

and specify that from now on, data should no longer be used to contain the originating error.

function sqlQuery(query, values) {
  try {
    // Execute some SQL
  } catch (err) {
    throw internal('Failed to query DB', { code: "sql-query-error", query, values }, { cause: err })
  }
}

devinivy commented 1 year ago

I think that's a great idea. If we can limit the blast radius by keeping data for non-error information as you've suggested, that would be nice. It's going to be a little tricky for server errors, which do often explicitly use data as the original error. That arguably is part of hapi's public interface (e.g. errors passed to failAction), so the data vs. cause delineation may need to just be the suggested usage for now, and we can sync hapi up with it in the next major.

kanongil commented 1 year ago

I would definitely like to see cause supported. We will have to be careful to not rely on the Error constructor option for this until node 16.10+ is required. For node 14 it is just a custom property.

I'm not especially a fan of the new proposed syntax, which will require you to pass a null option when no data is needed. More critically, it breaks for helper methods that take additional options.

Ideally it should look like a regular Error, eg. throw badImplementation('ERROR', { cause: err }); The options object could allow a data option, similar to new Boom() and boomify(). This will also allow the helpers methods to use the decorate option, and add helper specific options.

This will obviously be a breaking change. However, the change is limited, as it will only affect boom object creators that use helper methods with the data option. I only found one place where it is used in hapi core. From a consumer POV, the boom error will just have a new property.

Btw. on supported node releases, you should already be able to do something like:

throw badImplementation(new Error('Stuff should not throw an error', { cause: err }));

matthieusieben commented 1 year ago

I personally rely quite a lot on helpers and the optional data argument, so a breaking change will cause a little bit of work on my side. But this is not really problematic as the change is very straightforward and breaking changes are sometimes inevitable.

Additionally, changing the helper's second argument to behave like the constructor's options could allow to also specify a custom ctor, which is something I already wanted for a while (this allows creating custom helpers based on Boom helpers rather than having to use new Boom with a statusCode). So I second this proposition.

What should happen with unauthorized which is the only helper that does not have a data argument ?

unauthorized(message, scheme, attributes)

matthieusieben commented 1 year ago

Should all helpers have the following signature?

type helper<T = any> = (message: string, options: Options<T>) => Boom<T>

Which means that the 3 helpers that do not currently have only message and data arguments would be re-written:

export function unauthorized<Data>(message: string, options: Options<Data> & { scheme?: string; attributes?: string | unauthorized.Attributes }) => Boom<Data>

export function methodNotAllowed<Data>(message: string, options: Options<Data> & { allow?: allow?: string | string[] }) => Boom<Data>

export function internal<Data>(message, options: Options<Data>)

kanongil commented 1 year ago

Your suggestion seems very sensible (except that all args should be optional):

type HelperMethod<T = any> = (messageOrError?: string | Error, options?: Options<T>) => Boom<T>;

For unauthorized() the current lack of a data option is not due to it being computed, but I guess rather a matter of an API design choice. As such, it would make sense to allow it in an updated API.

moroine commented 1 year ago

Do we plan to have this soon? If we agree on an implementation I'd be happy to help writing the PR

hapijs / boom