feat: add TypeScript types

[Marsup]

I guess the title says it all. This was a convoluted one, I hope I didn't miss anything.

Also, tests don't pass locally, but show a weird error without details like shown in the screenshot, so I expect the CI to fail.

[kanongil]

The weird error comes when one of the test lines throw an error.

[Marsup]

Got most of them, but it's hard since the line is constantly wrong.

[Marsup]

Oh, I think I got this one, the last batch of expect.errors actually expect an error (duh), but it's only a type error, not a runtime error, so it either fails for the lack of runtime error, or for the typings error if I test it with expect.type. Should I just remove them entirely?

[kanongil]

This is an initial pass over the implementation. I did not look into the tests.

[kanongil]

I would allow the any type for the err parameter to match the type in catch (err) {}, allowing for simple usage without casting:

try {
    // whatever
} catch (err) {
    Bounce.rethrow(err, 'system');
    // do your thing...
}

[kanongil]

"languange" => "language"

[kanongil]

This comment line is redundant.

[kanongil]

Nice. I would probably have used function overloads to handle this, but this seems cleaner.

[kanongil]

Error is incorrect, it should be ErrorConstructor. This doesn't actually change anything, since Record<any, any> matches both Error and ErrorConstructor.

[kanongil]

Record<any, any> does not seem appropriate, as it is intended to construct an object type using keys from another object. I would do a simple: { [key: PropertyKey]: any }.

[kanongil]

Again, Record<any, any> seems to be used wrong. I would do { [key: string]: any }.

[kanongil]

Should be Omit<BounceOptions, 'return'>.

[kanongil]

Yay for using type predicates 👍

[kanongil]

Hmm, this can cause issues since a match removes the original type which might be more specific. I would add an overload to handle this:

export function isBoom(err: Boom): boolean;
export function isBoom(err: unknown): err is Boom;

We could even make it return true rather than boolean, but I would probably play it safe and do it like above.

[kanongil]

The predicate is missing the Hoek.Error type.

In reality, I would just return a simple boolean. Consumers would need to narrow it further anyway.

[kanongil]

Actually, I think this is the better:

export function isSystem(err: Error): boolean;
export function isSystem(err: unknown): err is Error;

[kanongil]

This also downgrades the typing for the errors when it matches and needs an overload to handle this case:

export function isError(err: Error): boolean;
export function isError(err: unknown): err is Error;

[kanongil]

When dealing with typescript, another helper method might be handy. Something like Bounce.assert(err), which will throw a system error if err is not instanceof Error. It would have a signature like:

function assert(err: unknown): asserts err is Error;

Then you could use it in typescript to ensure a catched error is really an error:

try {
    // whatever
}
catch (err) {
    Bounce.assert(err);
    // err now has the Error type
}

It could also be added as another option to rethrow() / ignore(), maybe even as a default in a future breaking release:

try {
    // whatever
}
catch (err) {
    Bounce.rethrow(err, 'system', { assert: true });
    // err now has the Error type
}

[hueniverse]

I feel this module would be better off converted to TS rather than being added types to...

[kanongil]

@hueniverse Yeah, I see that you have converted some of your modules. I guess you have changed your stance on authoring modules in Typescript?

While the TS code conversion is probably simple, especially with typings ready, publishing it is more complex. There aren't any established rules / procedures inside hapijs, and many ways to go about it. We would also need to update the linting config with typescript aware rules, like I have tried to do in hls-playlist-reader.

This module is probably a decent entry point for an initial typescript conversion, given the simple implementation. But someone would need to champion this with the blessing of the TSC.

[Nargonath]

I believe our initial stance was since not much people inside the TSC currently work with TypeScript we didn't want to further complicate the org maintenance. Also it felt kind of weird to have only a couple of packages migrated to TS within the org. We were dubious that hapi would ever be rewritten to TS considering the dynamic nature of hapi's API which might not sit well with TS type system without altering the framework API. Add to that the sheer amount of work such rewrite would represent, nobody in the TSC was interested in such project at that time.

I don't think this is written in stone though. I agree Bounce/Hoek are good candidates as they're hapi agnostic packages. I can bring the subject back to the rest of the TSC and see what is everyone's point of view. Opinions may have evolved since.