throttler
throttler copied to clipboard
Block duration option for throttler
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
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?
Yes. This will be a breaking change at the library level.
I think it's not possible to keep the current API.
@jmcdo29 This would be nice to have
@asif-jalil I look forward to this feature
@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!
@jmcdo29
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 is there any update?
@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
@jmcdo29 Looking good from my side. Up to you to merge whenever you feel like.
@jmcdo29 is there any update for merge?
Hey, this change will be released soon?
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
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 I have merged the recent update with this branch