add support for redis v4

[rahil-p]

Related: #141, #167

This pull request implements RateLimiterRedisNext, which is intended to be exclusively compatible with redis v4.x.

[animir]

@rahil-p Hey, this is great!

However, we can't create other class for this case. This will be confusing for everyone except you and me. Support for node-redis v4 package must be implemented in the same RateLimiterRedis class. If it is impossible to keep compatibility with old versions of node-redis, it is fine, we can release major version of rate-limiter-flexible, but it should support ioredis as it is now.

[animir]

@rahil-p I believe, RateLimiterRedis can be improved for redis v4+ with additional 5-6 if/else conditions. Is it possible to check if it is redis package and version is 4+ ? If yes, it would be good.

Yes, it would be not the best code structure and agains some best practice, but rate-limiter-flexible users would be glad for the result not code :)

[rahil-p]

@animir Thanks for the feedback. I agree that that's the better solution for users of the package.

I'm not sure what the best way to approach checking if the provided client is from redis v4. I can dig in and follow up - suggestions are welcome.

[animir]

@rahil-p If there is no way to get node-redis client version, storeType limiter option can be used to set it. It could be set to redis4+ and then checked in _upsert and other functions, where redis client used.

EDIT: After some thinking storeType would be confusing option. It is better to add a new option and call it storeClientVersion. For this particular case, it can be number 4. In the future it could support string version in x.y.z format or even with rules like in NPM.

[marlonelima]

Something new?

[animir]

@marlonelima Nothing the for the last month. If you wish to help, you can look at these changes and implement redis package v4+ support. Let us know.

[rahil-p]

I pushed changes in d110583 that need review. My availability has been limited, so I haven't been able to manually test out these changes. I'd appreciate if someone could give that a go.

[a-r-d]

@rahil-p I was just trying to test your changes and I got this error

    ERR Error running script (call to f_a1de965f42e177aa339706bd580f049065eca79a): @user_script:1: @user_script: 1: Lua redis() command arguments must be strings or integers

I did set the client store version to 4:

I really do want to use change though, I think it would be really nice since the 3.x version of redis module is very out of date and doesnt even have proper typescript types.

[manzan46]

👋 For what it's worth I just tested this PR and it's working with a basic rate limiting setup, so I don't think it's far off ready and would be really useful to have it merged (especially as it seems it doesn't bring breaking change thanks to clientStoreVersion)

    blockDuration: 30,
    duration: 10,
    points: 20,
    keyPrefix: 'rate-limite-key',
     keyFn: req => req.userId,

might be worth also adding a section to the Redis wiki to add clientStoreVersion as options when creating the Rate limiter:

    const rateLimiterRedis = new RateLimiterRedis({
     storeClient: redisClient, // same as now
     ... // all the classic options
     clientStoreVersion: 4,
   });
   ```

[animir]

@manzan46 Hi, a-r-d tested it and found an error a couple of months ago. There were no changes since then. Also, new code requires new tests to be written. Unfortunately, It can't be merged without tests. While this PR looks promising, it can't be merged like this.

[manzan46]

@manzan46 Hi, a-r-d tested it and found an error a couple of months ago. There were no changes since then. Also, new code requires new tests to be written. Unfortunately, It can't be merged without tests. While this PR looks promising, it can't be merged like this.

No problems at all, it was mainly to mention on basic usecase I didn't have any issue on my side, and try to get the ball rolling 😅 (definitely better to merge this with test 👌🏻 )

[animir]

Closing this PR. Changes arrived in the scope of other PR https://github.com/animir/node-rate-limiter-flexible/pull/218. Thank you.

[animir]

Support of ioredis v4+ added in 3.0.0.