Bind sendCommand to the current instance #227
Conversation
gamemaker1
left a comment
gamemaker1
left a comment
There was a problem hiding this comment.
LGTM apart from the one missing test. Thank you :)
| expect(result.totalHits).toEqual(1) | ||
| }) | ||
|
|
||
| it('should bind sendCommand to this', async () => { |
There was a problem hiding this comment.
Could you please add a similar test for sendCommandCluster?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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
source/lib.ts
Outdated
| // 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) |
There was a problem hiding this comment.
This isn't a huge deal, but could we bind sendCommand & sendCommandCluster once rather than every time they're called?
There was a problem hiding this comment.
I pushed a new commit for binding the given sendCommand before using it inside the wrapper func.
The sendCommandCluster function is already bound once.
nfriedly
left a comment
nfriedly
left a comment
There was a problem hiding this comment.
One more little thing, but otherwise I think this is good to go
| class CustomRedisStore extends RedisStore { | ||
| constructor() { | ||
| super({ | ||
| sendCommand: customSendCommand, | ||
| }) | ||
| } | ||
| } | ||
| const store = new CustomRedisStore() |
There was a problem hiding this comment.
Sorry I missed this yesterday, but I don't think we need the new class here (?)
I think this would work:
| class CustomRedisStore extends RedisStore { | |
| constructor() { | |
| super({ | |
| sendCommand: customSendCommand, | |
| }) | |
| } | |
| } | |
| const store = new CustomRedisStore() | |
| const store = new RedisStore({ | |
| sendCommand: customSendCommand, | |
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, I understand what you're doing better now. I think I can go with this.
test/store-test.ts
Outdated
| class CustomRedisClusterStore extends RedisStore { | ||
| constructor() { | ||
| super({ | ||
| sendCommandCluster: customSendCommandCluster, | ||
| }) | ||
| } | ||
| } | ||
| const store = new CustomRedisClusterStore() |
There was a problem hiding this comment.
Ditto above
| 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.
The same as above.
There was a problem hiding this comment.
I have removed these lines of code because the usage of this keyword using a subclass is being presented in the previous test.
kbarbounakis
deleted the
226-bind-sendCommand-to-the-current-instance
branch
3 participants
This PR closes #226 and binds
sendCommand, passed as parameter of theRedisStoreconstructor, to the current instance. This operation allowssendCommandmethod to usethiskeyword, if this is required.