hapijs / bounce

Selective error catching and rewrite rules
Other
176 stars 13 forks source link

feat: add TypeScript types #34

Open Marsup opened 2 years ago

Marsup commented 2 years ago

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. image

kanongil commented 2 years ago

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

Marsup commented 2 years ago

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

Marsup commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

kanongil commented 2 years ago

@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 commented 2 years ago

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.