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.

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

Jump to bottom

feat(guard): add method for overriding throttling message #1388

Closed
wants to merge 1 commit into from
Closed

feat(guard): add method for overriding throttling message #1388

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/throttler.guard.ts
View file
Open in desktop
Expand Up @@ -115,6 +115,13 @@ export class ThrottlerGuard implements CanActivate {
return md5(`${prefix}-${suffix}`);
}

/**
* Get throttling message
*/
protected throttlingMessage(): string {
return this.errorMessage;
}

/**
* Throws an exception for the event that the rate limit has been exceeded.
*
Expand All @@ -124,6 +131,6 @@ export class ThrottlerGuard implements CanActivate {
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
protected throwThrottlingException(context: ExecutionContext): void {
throw new ThrottlerException(this.errorMessage);
throw new ThrottlerException(this.throttlingMessage());
Copy link
Member

Choose a reason for hiding this comment

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

The error message itself isn't private so any child class can already override it. Any reason to make a method to get it and override it instead of just overriding the message itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this would make sense when we are doing i81n stuff

Copy link
Author

Choose a reason for hiding this comment

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

I think overriding method is better as oppose overriding the property directly incase there's an extra check/validation before displaying throttling message @jmcdo29

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. This shouldn't be breaking, so would you like to add a minor changeset to the PR for a release?

Copy link
Author

Choose a reason for hiding this comment

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

@jmcdo29 where do I add the minor changeset? CHANGELOG?

Copy link
Member

Choose a reason for hiding this comment

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

Run yarn changeset and follow the wizard

}
}