throttler icon indicating copy to clipboard operation
throttler copied to clipboard

Block duration option for throttler

Open asif-jalil opened this issue 1 year ago • 15 comments

PR Checklist

Please check if your PR fulfills the following requirements:

  • [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
  • [x] Tests for the changes have been added (for bug fixes / features)
  • [x] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • [ ] Bugfix
  • [x] 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, when a user exceeds their request limit within a defined time window (ttl), their requests are blocked for a fixed duration tied to the ttl.

Issue Number: 1660

What is the new behavior?

I introduced a new option called blockDuration when importing the throttler module option. If users choose not to specify blockDuration, the system will fall back to the default behavior, which relies on the ttl.

Does this PR introduce a breaking change?

  • [ ] Yes
  • [x] No

Other information

asif-jalil avatar Sep 08 '23 21:09 asif-jalil

From a quick pass it looks pretty good, I'll need to go more in depth later.

One main concern of mine is that this will be a breaking change at the library level, so anyone who maintains a storage package will need to update to the new interface.

Is there any way to keep the current API, or is hat not possible with the nature of the changes?

jmcdo29 avatar Sep 08 '23 21:09 jmcdo29

Yes. This will be a breaking change at the library level.

I think it's not possible to keep the current API.

asif-jalil avatar Sep 08 '23 22:09 asif-jalil

@jmcdo29 This would be nice to have

apmovamo avatar Dec 03 '23 11:12 apmovamo

@asif-jalil I look forward to this feature

cdx111 avatar Feb 01 '24 03:02 cdx111

@asif-jalil I look forward to this feature

@cdx111 I also look forward to this feature. But I don't know why @jmcdo29 pushed this to a long waiting list!

asif-jalil avatar Feb 01 '24 05:02 asif-jalil

@jmcdo29

cdx111 avatar Feb 02 '24 08:02 cdx111

Apologies for the delay and thank you for your patience. It's been hectic keeping up with support and general life. I wanna revisit this and see what can be done with it, as I can tell there is a community want for this.

jmcdo29 avatar Feb 09 '24 18:02 jmcdo29

@jmcdo29 is there any update?

asif-jalil avatar Feb 18 '24 07:02 asif-jalil

@jmcdo29 Sorry for my late reply, I'm very busy lately.

I only have some minor requests in terms of docblock things. I'm not too much involved in the project but it looks fine to me for what I see. I also noticed the ttl is sometimes in millisconds and seconds, which seems inconsistent, so make sure that it is milliseconds in the right places whenever it is being mentioned that they are in ms rather than secs.

Other than that, I don't think I need to make any adjustments to my redis storage, if so, someone will probably run into an issue and submit an issue :P

kkoomen avatar Feb 27 '24 17:02 kkoomen

@jmcdo29 Looking good from my side. Up to you to merge whenever you feel like.

kkoomen avatar Feb 27 '24 19:02 kkoomen

@jmcdo29 is there any update for merge?

asif-jalil avatar Apr 17 '24 06:04 asif-jalil

Hey, this change will be released soon?

dancornilov avatar Jun 14 '24 14:06 dancornilov

Again, apologies for the delay. My personal life has been rather hectic as of late.

@asif-jalil I think the only other thing I could ask for for this is a changeset for a major release, as this does make an interface change to a publicly exposed API that could break integrations. The changeset should make it clear that consumers should be unaffected by this change, and that it is only storage library owners who need to worry about providing a compatible integration.

This can be generated using pnpm changeset and following the wizard

jmcdo29 avatar Jun 14 '24 14:06 jmcdo29

Okay, I think the recent minor change I merged in added some conflicts, but if you get those fixed up I'll get this merged soon after

jmcdo29 avatar Jun 25 '24 13:06 jmcdo29

@jmcdo29 I have merged the recent update with this branch

asif-jalil avatar Jun 27 '24 04:06 asif-jalil