microlinkhq / async-ratelimiter

Rate limit made simple, easy, async.
MIT License
318 stars 23 forks source link

1.2.8 to 1.3.0 contains breaking API changes released as a minor bump, shouldn't it have been 2.0? #29

Closed brandonros closed 3 years ago

brandonros commented 3 years ago
  // check if we are already rate limited without deducting
  const limit = await rateLimiter.get({
    id: key,
    max,
    duration,
    decrease: false
  })
  // if we are rate limited already, bail
  if (!limit.remaining) {
    warn('RateLimited', {
      data: {
        limit,
        level,
        key
      }
    })
    getMetric<Counter>('requests_rate_limited_total').labels(method, path).inc(1)
    return new UncachedHttpResponse('', {
      'X-Rate-Limit-Limit': limit.total,
      'X-Rate-Limit-Remaining': Math.max(0, limit.remaining - 1),
      'X-Rate-Limit-Reset': limit.reset
    }, 429)
  }
  // if we are not rate limited, deduct one from rate limit pool
  await rateLimiter.get({
    id: key,
    max,
    duration,
    decrease: true
  })
  getMetric<Counter>('requests_not_rate_limited_total').labels(method, path).inc(1)

totally broke this code...?

brandonros commented 3 years ago

from https://semver.org/

MAJOR version when you make incompatible API changes

MINOR version when you add functionality in a backwards compatible manner

PATCH version when you make backwards compatible bug fixes.

Kikobeats commented 3 years ago

I don't follow; what's the breaking change in your opinion?

brandonros commented 3 years ago

image

https://github.com/microlinkhq/async-ratelimiter/compare/v1.2.8...v1.3.0

You used to be able to call .get with decrease: false

Now it just does the decrement no matter what

Kikobeats commented 3 years ago

That's right and sorry for the misunderstanding, definitely it should have been launched as a major version.

I added a note over the releases to clarify that:

https://github.com/microlinkhq/async-ratelimiter/releases/tag/v1.3.0

Alternatively, you can always get the value without decrementing just by query against the Redis instance.

brandonros commented 3 years ago

might i recommend releasing a 2.0.0 just to save anybody else this heartache/pain