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: allow customizable error #58

Merged
merged 1 commit into from
Apr 3, 2024
Merged

feat: allow customizable error #58

merged 1 commit into from
Apr 3, 2024

Conversation

psibean
Copy link
Contributor

@psibean psibean commented Apr 2, 2024

Exposes the statusCode, message, and code parameters for the error initialization

This will fix issue #55

Exposes the statusCode, message, and code parameters
for the error initialisation
Copy link
Collaborator

@davidgonmar davidgonmar left a comment

Choose a reason for hiding this comment

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

Looks good to me. One thing I'd add is that, since you are changing the interface, to allow adding a factory function instead of the CsrfErrorConfig (one or the other). Something like:

export type CsrfErrorConfig = {
  statusCode: number;
  message: string;
  code: string | undefined;
} | {
  buildError: () => any;
}

It might be useful for some use cases. For example, in NestJS, it would allow a custom error that inherits their HttpException class (or just their ForbiddenException) (if I remember the name correctly haha), which is catched and handled automatically.

@psibean psibean merged commit 24ec3ab into main Apr 3, 2024
7 checks passed
@psibean
Copy link
Contributor Author

psibean commented Apr 3, 2024

Looks good to me. One thing I'd add is that, since you are changing the interface, to allow adding a factory function instead of the CsrfErrorConfig (one or the other). Something like:

Could be useful, it's also a non-breaking (backwards compatible) change so will consider it separately from this one. 👍🏻 It's typically what the error handler middleware should be setup to do in any case.

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.

2 participants