-
Notifications
You must be signed in to change notification settings - Fork 192
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Rework implementation with initial .cause
support
#304
base: next
Are you sure you want to change the base?
Conversation
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. |
964a429
to
9b9ed18
Compare
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 Here are a few questions I have:
|
Thanks for the feedback – those are all great questions.
I considered reverting this, especially after I started to use it internally. I'm very open to change it back.
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
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. |
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 |
Regarding the Specifically it does not seem possible to model with typescript, as in defining a |
Errors are generally used as thrown values. As such, strongly typing them does not make a lot of sense. Even the 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 |
c1ac3d4
to
4135963
Compare
4203898
to
bc92c37
Compare
I removed the |
There is no necessity to type it though. As long as the returned value matches the Boom interface (decorate can't override Boom properties such as data, output, status code, etc) the type contract will be respected. On the other end cause might be a good enough placeholder for most use cases. |
bc92c37
to
b2102b1
Compare
The I don't know how it is used in the wild, but the feature is not used within the hapijs org itself. I did find some use in the legacy "hawk" module. Notice that it already uses If you want to decorate your boom object, a better solution will probably be to subclass Boom and handle it using a custom option. |
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
(c346820). Makes the API interface simpler and more consistent.Object.assign()
.payload.attributes
property fromunauthorized()
(d277413). Nonsensical and never used.Added features
Error.cause
property (1813bef).boomify()
called on aBoom
error (1813bef).Error
in calls toboomify()
(9600c08). This allows any catched "error" to be safely passed.headers
option tonew Boom()
(ded25b4).I also found and fixed a bug in 2137697.
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.