Repository navigation
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-class Valkey support to @fastify/rate-limit by introducing a new valkey configuration option backed by a GLIDE-scripted ValkeyStore, plus documentation/examples and new test coverage.
Changes:
- Add
ValkeyStoreimplementation using@valkey/valkey-glidescripting APIs and wire it into plugin initialization viasettings.valkey. - Add runtime validation to enforce mutual exclusivity between
store,redis, andvalkey. - Extend TypeScript types, docs, examples, and add unit + integration tests for Valkey behavior.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
index.js |
Adds valkey store selection and validates mutually exclusive store options. |
store/ValkeyStore.js |
New GLIDE-backed store implementation using cached script invocation. |
types/index.d.ts |
Adds FastifyRateLimitValkeyClient, adjusts option types (redis to unknown, valkey added). |
types/index.test-d.ts |
Adds type-level coverage for valkey option and client interface. |
test/valkey-store.test.js |
Unit tests for ValkeyStore key prefixing, child behavior, caching, and error forwarding. |
test/valkey-rate-limit.test.js |
Plugin-level tests for valkey option and new mutual exclusivity validation. |
test/valkey-real-rate-limit.test.js |
Real integration tests against a running Valkey backend (GLIDE client). |
example/example-valkey-direct.mjs |
Example using a directly managed GLIDE client. |
example/example-fastify-valkey-glide.mjs |
Example using @fastify/valkey-glide to supply the resolved client. |
README.md |
Documents Valkey usage, installation guidance, and examples. |
package.json |
Adds optional peer + dev dependency on @valkey/valkey-glide and a local valkey docker script. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Signed-off-by: Jani Mikkonen <jani.mikkonen@qvik.fi>
a1e996f to
fad6b16
Compare
|
I’ve rebased this PR onto the current main and substantially reworked the implementation based on the earlier feedback. The updated approach uses GLIDE’s Script and invokeScript APIs, keeps GLIDE optional for users who do not use Valkey, and runs the real Valkey integration test only when VALKEY_HOST is available. The PR description also documents the shared GLIDE installation requirement and proposes a reusable Fastify Valkey CI workflow instead of testing Valkey behavior against Redis. I’d appreciate feedback on the public valkey option, the script lifecycle approach, and the proposed CI direction when you have time. |
Eomm
left a comment
There was a problem hiding this comment.
Thanks for the proposal!
I think we’ve discussed a similar idea in the past, and I’d prefer not to add Valkey Glide support directly to fastify-rate-limit.
My main concern is keeping the plugin focused and avoiding additional maintenance responsibilities for specific storage clients. We already have a dedicated integration for Valkey Glide in the Fastify ecosystem: https://github.com/fastify/fastify-valkey-glide
I think there are a couple of good alternatives that would keep things more modular:
- Fork and create a dedicated
fastify-valkey-limitplugin for users who want a first-class integration with Valkey Glide. - Develop a standalone Valkey store module that can be used with
fastify-rate-limit(likeyish I did for this validator https://github.com/Eomm/joi-compiler)
This way, users who need this specific integration can have it without adding more complexity to the core plugin, but we may add it as reference in the readme
Overall, I think this functionality would be better maintained as a separate integration rather than included directly in fastify-rate-limit.
|
Thanks for the feedback. Keeping fastify-rate-limit focused and avoiding extra maintenance for specific storage clients makes complete sense. I don't currently have the free time to create and maintain a standalone store module or separate plugin, so I will close this PR. If anyone else wants to build that package, please feel free to use or adapt any of the code from this PR if it is still useful. |
Why
Applications using Valkey through GLIDE should be able to share their existing client with
@fastify/rate-limit, instead of maintaining a separate Redis connection or implementing a custom store.Closes #436.
Related: fastify/fastify-valkey-glide#33
What changed
valkeyoption accepting either aGlideClientorGlideClusterClient.ScriptandinvokeScriptAPIs. GLIDE therefore handles script loading, caching, and cache-miss recovery; no rawcustomCommandfallback is maintained.storeprecedence unchanged and reject simultaneousredisandvalkeyoptions when no custom store is provided.@valkey/valkey-glide^2.0.1, matching@fastify/valkey-glide.GLIDE installation
GLIDE associates
Scriptinstances with its module-level script registry. The supplied client and the scripts therefore need to resolve from the same application-level@valkey/valkey-glideinstallation.The README installs
@valkey/valkey-glideat the application root alongside@fastify/valkey-glide. This is the supported configuration. The implementation intentionally does not add recovery machinery for applications loading multiple physical GLIDE copies.Valkey CI follow-up
Fastify currently provides a reusable Redis workflow through
plugins-ci-redis.yml, but no equivalent Valkey workflow.The real Valkey integration test in this PR is therefore skipped unless
VALKEY_HOSTis set. It does not useREDIS_HOSTor run against the Redis service: that would validate Redis compatibility rather than Valkey, and the Redis 5 lane is below GLIDE's supported Redis versions.Recommended follow-up:
plugins-ci-valkey.ymltofastify/workflows, based on the existing reusable Redis workflow but provisioning actual Valkey.VALKEY_HOST.@fastify/valkey-glide.Until that workflow exists, the Valkey store and plugin tests continue to run in normal CI, while the real service test remains visibly skipped.
Validation
VALKEY_HOST=127.0.0.1 npm testGlideClusterClient