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

fix: export interface version of HttpException #56

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

Conversation

mc2147
Copy link

@mc2147 mc2147 commented Mar 24, 2023

No description provided.

@mc2147 mc2147 linked an issue Mar 24, 2023 that may be closed by this pull request
import { z } from "zod"
import { HTTPMethods } from "../with-route-spec/middlewares/with-methods"
import { SecuritySchemeObject, SecurityRequirementObject } from "openapi3-ts"

/** Export an interface version of HttpException to avoid class inheritance issues */
export interface IHttpException
Copy link
Author

@mc2147 mc2147 Mar 24, 2023

Choose a reason for hiding this comment

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

Do we want to name this IHttpException to keep the naming consistent with the base class, or IHttpError?

I also tried the suggested example from the github issue and it doesn't seem like KnownDeviceError can extend this interface as it is right now:
image

Is the intention to consolidate the two types by adding status, metadata, options to KnownDeviceError?

Copy link
Contributor

Choose a reason for hiding this comment

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

yep I believe so

class-based usage is discouraged so I'd say IHttpError is better to stay consistent with the error interfaces on Seam Connect, or maybe even just HttpError

Copy link
Author

@mc2147 mc2147 Mar 24, 2023

Choose a reason for hiding this comment

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

@seveibar @codetheweb just to confirm, is this what we're thinking for the IHttpError interface?
image

We might have to add some attributes to seam-connect entities to implement this interface as shown in the screenshot.

For comparison, this is the existing HttpException class in nextlove:

declare class HttpException extends Error {
    status: number;
    metadata: HttpExceptionMetadata;
    options: ThrowingOptions;
    constructor(status: number, metadata: HttpExceptionMetadata, options?: ThrowingOptions);
    toString(): string;
}

I need to take off for an appointment soon, but if we can get consensus on an approach I will merge later!

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, I think we need metadata as well but otherwise looks good

import { z } from "zod"
import { HTTPMethods } from "../with-route-spec/middlewares/with-methods"
import { SecuritySchemeObject, SecurityRequirementObject } from "openapi3-ts"

/** Export an interface version of HttpException to avoid class inheritance issues */
export interface IHttpException
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export interface IHttpException
export interface IHttpException {
is_http_exception: true,
status: number
message: string
}

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.

Add Support for IHTTPError (error interface)
3 participants