Skip to content

Bind sendCommand to the current instance #227

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nfriedly merged 4 commits into from
Nov 30, 2025
Merged

Bind sendCommand to the current instance #227

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdfaa69
bind sendCommand to the current instance
kbarbounakis Nov 29, 2025
bccebdc
bind sendCommand once
kbarbounakis Nov 30, 2025
0df5b66
test sendCommandCluster binding
kbarbounakis Nov 30, 2025
f629438
update sendCommand binding test
kbarbounakis Nov 30, 2025
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
4 changes: 2 additions & 2 deletions source/lib.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ export class RedisStore implements Store {
if ('sendCommand' in options && !('sendCommandCluster' in options)) {
// Normal case: wrap the sendCommand function to convert from cluster to regular
this.sendCommand = async ({ command }: SendCommandClusterDetails) =>
options.sendCommand(...command)
options.sendCommand.bind(this)(...command)
Copy link
Member

@nfriedly nfriedly Nov 30, 2025

Choose a reason for hiding this comment

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

This isn't a huge deal, but could we bind sendCommand & sendCommandCluster once rather than every time they're called?

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 pushed a new commit for binding the given sendCommand before using it inside the wrapper func.

https://github.com/express-rate-limit/rate-limit-redis/pull/227/files#diff-5c2516e9822679491492c832b9ace79b6945bdd891586e07b0232ebef1c7e0b1R97

The sendCommandCluster function is already bound once.

} 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
27 changes: 27 additions & 0 deletions test/store-test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,4 +345,31 @@ 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

@gamemaker1 gamemaker1 Nov 30, 2025

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

@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.

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

@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.

@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

@nfriedly nfriedly Nov 30, 2025

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

@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.

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 •
edited
Loading

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

@nfriedly nfriedly Nov 30, 2025

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)
})
})