feat: node-redis #413
feat: node-redis #413
Conversation
|
This already looks pretty great :) |
| @@ -62,3 +62,4 @@ RedisStore.prototype.child = function (routeOptions) { | |||
| } | |||
|
|
|||
| module.exports = RedisStore | |||
| module.exports.lua = lua | |||
There was a problem hiding this comment.
Need lua script for NodeRedisStore instead of copy&paste. Both of those should always be the same for all Redis clients.
Should we move lua script to other place instead?
test/node-redis-rate-limit.test.js
Outdated
There was a problem hiding this comment.
Copy pasted tests for ioredis... should rather just reuse tests and run them with other client instead?
Will make comments as there are many things we might want to change... just started with the initial PR and can then discuss how we want to do it. |
Co-authored-by: Gürgün Dayıoğlu <[email protected]> Signed-off-by: Aleš Blaznik <[email protected]>
| if (!this.redis.rateLimit) { | ||
| throw new Error( | ||
| 'rateLimit script missing on Redis instance. Add it when creating client: ' + | ||
| 'const redis = createClient({ scripts: { rateLimit: rateLimit.NodeRedisStore.rateLimitScript }})' | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
My only gripe with this is that we now expose the name of the script while with ioredis we'd load it internally. While the idea of injecting your own script seems cool, it currently seems a little inconsistent to me
Can't we initialize or use the script after the client is created?
.eval seems like an option:
There was a problem hiding this comment.
Agree it's not consistent. I've not seen this eval as a possible solution... will try it out if this will work out and update the PR.
Thanks for the reference 🙏🏻
| */ | ||
|
|
||
| const { lua } = require('./RedisStore') | ||
| const { defineScript } = require('@redis/client') |
There was a problem hiding this comment.
I would not add a depedency to @redis/client here. we should be taking an instance of that in the constructor instead.
There was a problem hiding this comment.
Yeah same as #413 (comment)
The goal is to not depend on the lib
Only the client that's passed should be used
Hi guys 👋🏻! This is mine first contribution to this project so will need some help 🙏🏻 .
We already use this library in production and found #368 where people are interested in
node-redissupport. Also read the suggestions about refactoring this library to make it easier to extend to other stores, but that would be too much for now.Understand this isn't the ideal way of adding
node-redis, but if we're happy with the first iteration and at least people are able to usenode-redisthat would be great.With
node-redis, one will need to initialise client like this (will update the docs):👆🏻I explored
node-redisa bit if there is a way to define script on already connected client (to have the setup same for ioredis and node-redis), but didn't find any solution... will have another round...Checklist
npm run testandnpm run benchmarkand the Code of conduct