-
-
Notifications
You must be signed in to change notification settings - Fork 36
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.
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
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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. | ||||||||||||||||||||||||
|
|
@@ -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 () => { | ||||||||||||||||||||||||
| // 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
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have updated
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
| class CustomRedisClusterStore extends RedisStore { | |
| constructor() { | |
| super({ | |
| sendCommandCluster: customSendCommandCluster, | |
| }) | |
| } | |
| } | |
| const store = new CustomRedisClusterStore() | |
| const store = new RedisStore ({ | |
| sendCommandCluster: customSendCommandCluster, | |
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The same as above.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.tsis missing a mock function -and some tests- forsendCommandCluster. Have you prepared such a method? It seems thatsendCommandClusteroption should be tested too. Maybe in a different PR?There was a problem hiding this comment.
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
sendCommandClusterbinding. It's a simple test using the already implemented mocksendCommand.ioredis-mockprovides aClustermock class -for future use- but it does not follow the specification of a redis cluster commandhttps://github.com/stipsan/ioredis-mock/?tab=readme-ov-file#clusterexperimental