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: version 5.0.0 #1565

Merged
merged 22 commits into from
Sep 5, 2023
Merged

feat: version 5.0.0 #1565

merged 22 commits into from
Sep 5, 2023

Conversation

jmcdo29
Copy link
Member

@jmcdo29 jmcdo29 commented Jul 6, 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?

Currently we cannot have multiple throttler contexts

Issue Numbers:

ref: #1369
ref: #1522
ref: #1443
ref: #1388
ref: #1492
ref: #1089
ref: #1221
ref: #894

What is the new behavior?

This is a bit of something that I've wanted to do for a while and inspired by this pr. With the new approach, we're now able to let users define scales at which they would like the throttling to work over, and let it work for any number of configurations, from a single 10 requests in 5 seconds to scales of months, or milliseconds

Does this PR introduce a breaking change?

  • Yes
  • No

It's worth noting there are quite a few breaking changes in this which will be reflected in the changelog as well, but better to have multiple mentions in my opinion

  • ttl is now in milliseconds, not seconds, but there are time helper exposed to ease the migration to that
  • the module options is now either an array or an object with a throttlers array property
  • @Throttle() now takes in an object instead of two parameters, to allow for setting multiple throttle contexts at once in a more readable manner
  • @ThrottleSkip() now takes in an object with string boolean to say which throttler should be skipped

Other information

docs pr

@jmcdo29 jmcdo29 requested a review from kkoomen July 6, 2023 18:02
@jmcdo29
Copy link
Member Author

jmcdo29 commented Jul 6, 2023

@Ganesha2552 this PR is heavily inspired by your approach with my own take on it. Please feel free to review it and see what you think. I believe this has a bit more freedom about it on defining multiple scales to work with

@jmcdo29
Copy link
Member Author

jmcdo29 commented Jul 6, 2023

@benjlevesque if I could request you look over this too, as I believe I merged your changes in correctly and handled them as necessary, but want a second set of eyes on it

@jmcdo29 jmcdo29 force-pushed the feat/mutliple-throttlers branch from ce555d7 to d3749ac Compare July 7, 2023 13:05
@weshuiz
Copy link

weshuiz commented Jul 7, 2023

can i qucikly mention that the skip option could also be named forRoot/ forRootAsync as people will be already familiar with the phraze of it

@jmcdo29
Copy link
Member Author

jmcdo29 commented Jul 7, 2023

@weshuiz I'm not sure I understand what you mean here. ThrottlerModule has forRoot and forRootAsync

@weshuiz
Copy link

weshuiz commented Jul 7, 2023

@jmcdo29 it was more of a name suggestion for the skip option
since it already exist, i gues never mind about this

@benjlevesque
Copy link
Contributor

@jmcdo29 looks all right for my part👍

This change seems super useful, congrats!
Have you considered keeping a retro-compatible API for the @Throttle decorator, to keep upgrade simple and code tidy?

@jmcdo29
Copy link
Member Author

jmcdo29 commented Jul 7, 2023

@benjlevesque I've thought about it, but decided against it. Maintaining the backwards compatible @Throttler() API would be a burden, and while it shouldn't really be much of an issue, I figure just make it like a band-aid and rip it all off at once

@jmcdo29 jmcdo29 changed the title feat: allow for multiple throttler contexts feat: version 5.0.0 Jul 22, 2023
@jmcdo29 jmcdo29 force-pushed the feat/mutliple-throttlers branch from d3749ac to f9468b1 Compare September 4, 2023 17:44
ImChrisChen and others added 2 commits September 4, 2023 10:55
This is a bit of something that I've wanted to do for a while and inspired by
[this pr][pr]. With the new appraoch, we're now able to let users define scales
at which they would like the throttling to work over, and let it work for any
number of configuratins, from a single 10 requests in 5 seconds to scales of
months, or milliseconds

BREAKING CHANGES:

It's worth noting there are quite a few breaking changes in this which will be
reflected in the changelog as well, but better to have multiple mentions in my
opinion

* ttl is now in milliseconds, not seconds, but there are time helper exposed
to ease the migration to that
* the module options is now either an array or an object with a `throttlers`
array property
* `@Throttle()` now takes in an object instead of two parameters, to allow for
setting multiple throttle contexts at once in a more readable manner
* `@ThrottleSkip()` now takes in an object with string boolean to say which
throttler should be skipped

pr: #1522

ref: #1369
ref: #1522
This is a bit of something that I've wanted to do for a while and inspired by
[this pr][pr]. With the new appraoch, we're now able to let users define scales
at which they would like the throttling to work over, and let it work for any
number of configuratins, from a single 10 requests in 5 seconds to scales of
months, or milliseconds

BREAKING CHANGES:

It's worth noting there are quite a few breaking changes in this which will be
reflected in the changelog as well, but better to have multiple mentions in my
opinion

* ttl is now in milliseconds, not seconds, but there are time helper exposed
to ease the migration to that
* the module options is now either an array or an object with a `throttlers`
array property
* `@Throttle()` now takes in an object instead of two parameters, to allow for
setting multiple throttle contexts at once in a more readable manner
* `@ThrottleSkip()` now takes in an object with string boolean to say which
throttler should be skipped

pr: #1522

ref: #1369
ref: #1522
@jmcdo29 jmcdo29 force-pushed the feat/mutliple-throttlers branch 2 times, most recently from 3b8c3df to 7dbcb5d Compare September 5, 2023 06:35
@jmcdo29 jmcdo29 force-pushed the feat/mutliple-throttlers branch from 7dbcb5d to 29e3164 Compare September 5, 2023 06:37
@jmcdo29 jmcdo29 merged commit f579ced into master Sep 5, 2023
@jmcdo29 jmcdo29 deleted the feat/mutliple-throttlers branch September 5, 2023 06:42
@jmcdo29 jmcdo29 mentioned this pull request Sep 5, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants