Skip to content

fix: export interface version of HttpException #56

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.

Sign up for GitHub

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

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

fix: export interface version of HttpException #56

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/nextlove/src/types/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,15 @@
import { NextApiResponse, NextApiRequest } from "next"
import { Middleware as WrapperMiddleware } from "nextjs-middleware-wrappers"
import { HttpException } from "nextjs-exception-middleware"
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 •
edited
Loading

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 •
edited
Loading

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

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
}

/** Omitting metadata properties from base level attributes, TS is getting them confused */
extends Omit<HttpException, "name" | "message"> {}

export type Middleware<T, Dep = {}> = WrapperMiddleware<T, Dep> & {
securitySchema?: SecuritySchemeObject
securityObjects?: SecurityRequirementObject[]
Expand Down