Skip to content
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

feat: add TypeScript types #34

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: add TypeScript types #34

wants to merge 1 commit into from

Conversation

Marsup
Copy link
Contributor

@Marsup Marsup commented May 25, 2022

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
Copy link
Contributor

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

@Marsup
Copy link
Contributor Author

Marsup commented May 25, 2022

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

@Marsup
Copy link
Contributor Author

Marsup commented May 25, 2022

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?

Copy link
Contributor

@kanongil kanongil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @param types {@link BounceErrorTypes}
* @param options {@link BounceOptions}
*/
export function rethrow<TErr extends Error, TOpts extends BounceOptions>(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* A single item or an array of items of:
* - An error constructor (e.g. `SyntaxError`).
* - `'system'` - matches any languange native error or node assertions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"languange" => "language"


/**
* Throws the error passed if it matches any of the specified rules where:
* - `err` - the error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment line is redundant.

*/
export type BounceErrorTypes = BounceErrorType | BounceErrorType[];

export type BounceReturn<
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import type { Boom } from "@hapi/boom";
import type { AssertionError } from "assert";

export type BounceErrorType = Error | "system" | "boom" | Record<any, any>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

import type { Boom } from "@hapi/boom";
import type { AssertionError } from "assert";

export type BounceErrorType = Error | "system" | "boom" | Record<any, any>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* An object which is assigned to the `err`, copying the properties onto the error.
*/
decorate?: Record<any, any>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

operation: Function | Promise<any> | any,
action?: "rethrow" | "ignore",
types?: BounceErrorTypes,
options?: BounceOptions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be Omit<BounceOptions, 'return'>.

lib/index.d.ts Show resolved Hide resolved
*/
export function isSystem(
err: unknown
): err is
Copy link
Contributor

@kanongil kanongil May 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I think this is the better:

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

@kanongil
Copy link
Contributor

kanongil commented May 26, 2022

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
Copy link
Contributor

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

@kanongil
Copy link
Contributor

@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
Copy link
Member

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.

@Marsup Marsup mentioned this pull request Oct 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants