ratelimit-js icon indicating copy to clipboard operation
ratelimit-js copied to clipboard

APIs to reset consumed tokens and get remaining tokens (WIP)

Open sourabpramanik opened this issue 1 year ago • 12 comments

This PR adds two new APIs:

  • [x] Reset used tokens
  • [x] Get remaining tokens
  • [x] Add tests for both APIs

Feat: #80

sourabpramanik avatar Feb 29 '24 16:02 sourabpramanik

@sourabpramanik is attempting to deploy a commit to the Upstash Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Feb 29 '24 16:02 vercel[bot]

Looks good, and let's add a reset button to next.js example

ogzhanolguncu avatar Mar 04 '24 09:03 ogzhanolguncu

For the get remaining API, do you guys have any suggestions for the implementation? I am trying to figure out a way where every algorithm will have their own method to retrieve the remaining tokens because the data type for each algorithm is different. So please let me know if you have any better ideas.

sourabpramanik avatar Mar 04 '24 13:03 sourabpramanik

I believe that as long as the commands remains in Lua script, it should be acceptable. We don't want to cause more round-trips than necessary.

ogzhanolguncu avatar Mar 06 '24 07:03 ogzhanolguncu

@ogzhanolguncu @enesakar These are my proposed changes, the implementation of the algorithm does not change just that I have changed the Algorithm function return signature so that we can add get remaining tokens API for each different window as they use different methods to store tokens in Redis.

Changes applied to single region fixed window algorithm

sourabpramanik avatar Mar 10 '24 11:03 sourabpramanik

const ratelimit = new Ratelimit({
  redis: kv,
  limiter: {getRemaining: _,limit: _},
});

Why do we expose getRemaining and limit to outside when they can't even initialize it that way?

I think initializer config signature should be the same. It's a bit confusing both for the users and devs. Imagine firing up your intellisense and seeing getRemaining there, but you can't do anything, I believe it's just confusing.

We should find another way to append getRemaining to ratelimit instance

ogzhanolguncu avatar Mar 12 '24 07:03 ogzhanolguncu

The signature doesn't have to be like this it can work like it used to do by providing the limiter function. I will refactor this

sourabpramanik avatar Mar 12 '24 08:03 sourabpramanik

const ratelimit = new Ratelimit({
  redis: kv,
  limiter: {getRemaining: _,limit: _},
});

Why do we expose getRemaining and limit to outside when they can't even initialize it that way?

I think initializer config signature should be the same. It's a bit confusing both for the users and devs. Imagine firing up your intellisense and seeing getRemaining there, but you can't do anything, I believe it's just confusing.

We should find another way to append getRemaining to ratelimit instance

Yeah I actually missed the intellisense 😅

sourabpramanik avatar Mar 12 '24 08:03 sourabpramanik

const ratelimit = new Ratelimit({
  redis: kv,
  limiter: {getRemaining: _,limit: _},
});

Why do we expose getRemaining and limit to outside when they can't even initialize it that way?

I think initializer config signature should be the same. It's a bit confusing both for the users and devs. Imagine firing up your intellisense and seeing getRemaining there, but you can't do anything, I believe it's just confusing.

We should find another way to append getRemaining to ratelimit instance

@ogzhanolguncu How about this implementation?

sourabpramanik avatar Mar 13 '24 17:03 sourabpramanik

APIs are definitely much better. I like it

ogzhanolguncu avatar Mar 18 '24 10:03 ogzhanolguncu

Thanks a lot for the changes! I think the changes look good overall. I only made a small fix in 4c9002d.

Thanks @CahidArda

sourabpramanik avatar Apr 08 '24 15:04 sourabpramanik

I feel like we can leave the discussion about changing the API for another time.

Yes I believe we can create a new issue for the same and discuss it throughout

sourabpramanik avatar Apr 08 '24 15:04 sourabpramanik