Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions source/lib.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,10 +94,11 @@ export class RedisStore implements Store {

if ('sendCommand' in options && !('sendCommandCluster' in options)) {
// Normal case: wrap the sendCommand function to convert from cluster to regular
const sendCommandFn = options.sendCommand.bind(this)
this.sendCommand = async ({ command }: SendCommandClusterDetails) =>
options.sendCommand(...command)
sendCommandFn(...command)
} else if (!('sendCommand' in options) && 'sendCommandCluster' in options) {
this.sendCommand = options.sendCommandCluster
this.sendCommand = options.sendCommandCluster.bind(this)
} else {
throw new Error(
'rate-limit-redis: Error: options must include either sendCommand or sendCommandCluster (but not both)',
Expand Down
55 changes: 55 additions & 0 deletions test/store-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import MockRedisClient from 'ioredis-mock'
import DefaultExportRedisStore, {
RedisStore,
type RedisReply,
type SendCommandClusterDetails,
} from '../source/index.js'

// The mock redis client to use.
Expand Down Expand Up @@ -345,4 +346,58 @@ describe('redis store test', () => {
// With NEW script we expect a fresh window: hits=1 and ttl reset
expect(result.totalHits).toEqual(1)
})

it('should bind sendCommand to this', async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a similar test for sendCommandCluster?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gamemaker1
test/store-test.ts is missing a mock function -and some tests- for sendCommandCluster. Have you prepared such a method? It seems that sendCommandCluster option should be tested too. Maybe in a different PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gamemaker1 I have pushed a commit for testing sendCommandCluster binding. It's a simple test using the already implemented mock sendCommand. ioredis-mock provides a Cluster mock class -for future use- but it does not follow the specification of a redis cluster command
https://github.com/stipsan/ioredis-mock/?tab=readme-ov-file#clusterexperimental

// A custom sendCommand that verifies `this` is bound to the RedisStore instance
const customSendCommand = async function (
this: RedisStore,
...args: string[]
) {
if (!(this instanceof RedisStore)) {
throw new TypeError('this is not bound to RedisStore instance')
}

return sendCommand(...args)
}

class CustomRedisStore extends RedisStore {
constructor() {
super({
sendCommand: customSendCommand,
})
}
}
const store = new CustomRedisStore()
Comment on lines 368 to 380
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I missed this yesterday, but I don't think we need the new class here (?)

I think this would work:

Suggested change
class CustomRedisStore extends RedisStore {
constructor() {
super({
sendCommand: customSendCommand,
})
}
}
const store = new CustomRedisStore()
const store = new RedisStore({
sendCommand: customSendCommand,
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test case is being used here for approaching and presenting subclassing, where a class inherits RedisStore and encloses a custom sendCommand -as I have mentioned in the issue-. It's not really needed here. It might be removed.

Copy link
Contributor Author

@kbarbounakis kbarbounakis Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have updated sendCommand test case for understanding the usage of this binding while we are using a subclass of RedisStore.It now includes a more realistic case where decrement operation is not allowed for some reasons.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I understand what you're doing better now. I think I can go with this.

store.init({ windowMs: 60 } as Options)
const key = 'test-store'
const { totalHits } = await store.increment(key)
expect(totalHits).toEqual(1)
})

it('should bind sendCommandCluster to this', async () => {
// A custom sendCommand that verifies `this` is bound to the RedisStore instance
const customSendCommandCluster = async function (
this: RedisStore,
commandDetails: SendCommandClusterDetails,
) {
if (!(this instanceof RedisStore)) {
throw new TypeError('this is not bound to RedisStore instance')
}

return sendCommand(...commandDetails.command)
}

class CustomRedisClusterStore extends RedisStore {
constructor() {
super({
sendCommandCluster: customSendCommandCluster,
})
}
}
const store = new CustomRedisClusterStore()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto above

Suggested change
class CustomRedisClusterStore extends RedisStore {
constructor() {
super({
sendCommandCluster: customSendCommandCluster,
})
}
}
const store = new CustomRedisClusterStore()
const store = new RedisStore ({
sendCommandCluster: customSendCommandCluster,
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same as above.

Copy link
Contributor Author

@kbarbounakis kbarbounakis Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have removed these lines of code because the usage of this keyword using a subclass is being presented in the previous test.

store.init({ windowMs: 60 } as Options)
const key = 'test-store'
const { totalHits } = await store.increment(key)
expect(totalHits).toEqual(1)
})
})