Open kanongil opened 9 months ago
I need to do a little more in-depth review, but based on the description I agree with all of the choices made here. This is great, a much appreciated modernization of boom 👍
I second Devin here, thanks Gil for the thorough description of the work and for the work itself of course. I agree with what's described so far and I'll take some more time later on for a more thorough review.
As a regular user of Boom, I like this very much. This is a much more traditional way of creating objects and makes the code much easier to understand. Finally being able to use cause
out of the box is great !
Here are a few questions I have:
decorate
is both useful internally and for users of the lib. Why not keep this option in the constructor ?Error
constructor will be completely ignored on older runtimes. This means that in those runtimes the err.cause
prop won't be set. Having the cause
prop can however be useful for error reporting tools (e.g. Sentry
). Do you explicitly want to drop support for older runtimes ?Boom
as data
.Thanks for the feedback – those are all great questions.
- The ability to
decorate
is both useful internally and for users of the lib. Why not keep this option in the constructor ?
I considered reverting this, especially after I started to use it internally. I'm very open to change it back.
- The second argument of the
Error
constructor will be completely ignored on older runtimes. This means that in those runtimes theerr.cause
prop won't be set. Having thecause
prop can however be useful for error reporting tools (e.g.Sentry
). Do you explicitly want to drop support for older runtimes ?
I explicitly want to drop support for old node runtimes. Older browser runtimes, probably not. As it is, they would need some kind of polyfill. Alternatively, I guess a simple this.cause ??= cause
after the super()
call could fix this (though they would not see the full printed stack trace).
- Since this is a breaking change, would it not be better to include changes required for Add support for native
cause
#300 as part of the same discussion? The idea is that we probably want to avoid two breaking releases in a row and Add support for nativecause
#300 kinda makes it obsolete to use aBoom
asdata
.
I see those changes as a separate PR, that could very well go into the same breaking release.
Though I'm not sure they are worth the bother, once this is merged. new Boom()
and boomify()
are already done, so it would only be for the helpers. Here cause
is implicitly supported through the data
argument of the 500+
helpers, and you can always (except for unauthorized
and methodNotAllowed
) create a base object instead: new Boom(message, { statusCode, cause: err }
instead.
I added support for old web runtimes in 7c7f86c.
FYI, this exposed a coverage reporting issue in lab. It seems it doesn't handle the ??=
assignment operator, and reports full coverage even if the RHS logic is never called.
Regarding the decorate
option, I had a look at reverting it and found another reason to remove the feature.
Specifically it does not seem possible to model with typescript, as in defining a class Boom<Data, Decoration>
with extra properties from the Decoration
generic.
Regarding the
decorate
option, I had a look at reverting it and found another reason to remove the feature.Specifically it does not seem possible to model with typescript, as in defining a
class Boom<Data, Decoration>
with extra properties from theDecoration
generic.
Errors are generally used as thrown values. As such, strongly typing them does not make a lot of sense. Even the Data
generic will be lost once the error is thrown. I personally never had any use for it. My approach usually consist of using type assertions in catch block to properly narrow the content of the boom error:
type Foo = { bar: string }
declare const isFoo = (x: unknown): x is Foo
try {
// ...
} catch (err) {
if (isBoom(err) && isFoo(err.data)) {
// err is Boom<Foo> here
}
// Unexpected error
throw err
}
The same goes for decorations:
type Foo = { bar: string }
declare const isFoo = (x: unknown): x is Foo
try {
// ...
} catch (err) {
if (isBoom(err) && isFoo(err)) {
// err is Boom<unknown> & Foo here (decorated with `Foo`)
}
// Unexpected error
throw err
}
The following notation can be used to model decorations with Typescript:
interface Boom<Data = unknown> extends Error {
isBoom: true
data: Data
}
interface BoomConstructor {
new <Data, Decoration>(options: { data: Data; decorate: Decoration }): Boom<Data> & Decoration
readonly prototype: Boom;
}
declare const Boom: BoomConstructor
Note that this notation might actually be the proper way of typing things in the d.ts
file since isBoom
checks for an interface match rather than an actual instanceof
operation.
This PR features an extensive rework of the Boom internals. While it is quite expansive, it is mostly a refactor while trimming some less used features. As it is, no code changes are required to use this in hapi itself.
The main motivation for this, is to utilize the new
Error.cause
property toboomify()
existing errors without modifying or cloning the object. The non-standard modifying and cloning has both been a cause of errors over the years!new Boom()
now creates an actualBoom
error object (named"Boom"
), setting thecause
according to the options.boomify()
itself sets a passed error as thecause
of a createdBoom
object. This means that the printedstack
will be a composite of the place where theBoom
object is created, and the stack of thecause
, making debugging more powerful.Regular boom errors using a string message should largely be unaffected.
Removed features
boom.cause.<attr>
.Error
(c346820690a4f293d53b23152c628fab7e701bd8). Makes the API interface simpler and more consistent.Object.assign()
.payload.attributes
property fromunauthorized()
(d277413080612d20cddc6afe2d1b4c3cee1482ac). Nonsensical and never used.Added features
Error.cause
property (1813bef2587ecccbc5543590e0f08350841318a7).boomify()
called on aBoom
error (1813bef2587ecccbc5543590e0f08350841318a7).Error
in calls toboomify()
(9600c08e972e31e70d6aac9a0aa866021d93ef8f). This allows any catched "error" to be safely passed.headers
option tonew Boom()
(ded25b4325c3cca46918ec2ba69fe201a3205b81).I also found and fixed a bug in 2137697902bdebb0c718d8df76cf1e072f8aec05.
The new implementation should be cross-compatible with the current version in normal usage, allowing mix-and-match of versions which is a likely scenario in common deployments.
Note that while this PR means that Boom now uses and supports
Error.cause
, it does not enable it as an explicit option for the helpers, as suggested in #300. It is used implicitly though, inboomify()
and in the server error helpers when thedata
argument is a non-Boom error. Whether it makes sense to update the API, as suggested in #300 is considered future work.FYI, I have done a review across the hapi repos, and the only conflicting usage I found, was the one in cookie. Besides that I found multiple uses of the
decorate
option in mozilla/hawk, where they even also use theObject.assign()
alternative.This rework also fixes #291, fixes #300, and fixes #302.
TODO
The functionality is complete, and I would appreciate reviews. The only thing missing is updating the docs.