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(guard): add method for overriding throttling message #1388

Closed

Conversation

dammy001
Copy link

@dammy001 dammy001 commented Mar 9, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@@ -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

jmcdo29 added a commit that referenced this pull request Jul 6, 2023
jmcdo29 added a commit that referenced this pull request Jul 6, 2023
@jmcdo29
Copy link
Member

jmcdo29 commented Jul 6, 2023

Added to #1565

@jmcdo29 jmcdo29 closed this Jul 6, 2023
@jmcdo29 jmcdo29 mentioned this pull request Jul 6, 2023
12 tasks
jmcdo29 added a commit that referenced this pull request Jul 7, 2023
jmcdo29 added a commit that referenced this pull request Sep 4, 2023
jmcdo29 added a commit that referenced this pull request Sep 4, 2023
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.

3 participants