Add new sendCommandCluster option #223
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds a `sendCommandCluster` option as an alternative to `sendCommand`. It can be set to a function that is passed an object with the key and isReadOnly value, in addition to the command. This provides everything that node-redis needs in cluster mode, and is expandable with new fields if necessary to support other libraries in cluster mode. Additionally I tweaked the internals to always pass this information along - previously the key would be lost when loading the script, so it could inadvertenly get sent to the wrong redis server. Fixes #222 Fixes #207 Fixes #208
mt-ks
approved these changes
Nov 19, 2025
Collaborator
mt-ks
left a comment
mt-ks
left a comment
There was a problem hiding this comment.
Great work!, Tested manually on local.
gamemaker1
approved these changes
Nov 19, 2025
Member
gamemaker1
left a comment
gamemaker1
left a comment
There was a problem hiding this comment.
Looks good, thank you so much @nfriedly!
Thanks @mt-ks for testing it manually - perhaps we can turn that into an external test like we have in express-rate-limit? We could use the docker configuration from your comment here.
Member
Author
|
Cool, thanks for the reviews, I'm going to go ahead and ship this. But, additional tests are a good idea. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This adds a
sendCommandClusteroption as an alternative tosendCommand.It can be set to a function that is passed an object with the
keyandisReadOnlyvalue, in addition to thecommand. This provides everything that node-redis needs in cluster mode, and is expandable with new fields if necessary to support other libraries in cluster mode.Additionally I tweaked the internals to always pass this information along - previously the key would be lost when loading the script, so it could inadvertenly get sent to the wrong redis server.
Fixes #222
Fixes #207
Fixes #208
Tests are failing on my machine, even without these changes. Jest seems to think everything is commonjs and then chokes when it hits the first import or export statement. I'm putting this up as a draft to see if it works in CI.
Edit: it passed on CI, so I think there's just something wrong with my machine. Still, please give it a bit of extra scrutiny.