redis - refactor: v6 client swap, API docs, and test uniformity - #2062
redis - refactor: v6 client swap, API docs, and test uniformity#2062jaredwray wants to merge 4 commits into
Conversation
Remove listeners from a replaced Redis client (matching Valkey), reset PXAT detection on client swap, and rethrow connection errors from getMany and clear so throwOnConnectError is consistent. Clear the connection-timeout timer after a successful connect, strip namespace prefixes only from the start of a key, and share a KeyvRedisConnect type so createKeyv accepts cluster and sentinel options. Co-authored-by: Jared Wray <me@jaredwray.com>
Document v5-to-v6 migration, absolute expires on set/setMany, the default
:: namespace separator, and createClient({ url }) instead of a URI string.
Co-authored-by: Jared Wray <me@jaredwray.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2062 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 55 55
Lines 5074 5086 +12
Branches 822 822
=========================================
+ Hits 5074 5086 +12 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Order public properties under the constructor, keep private methods last, map Redis null to undefined, and document the Hookified event contract in the README. Co-authored-by: Jared Wray <me@jaredwray.com>
Use test() throughout, faker for keys and values, cover Hookified event wiring, and assert missing gets return undefined rather than null. Co-authored-by: Jared Wray <me@jaredwray.com>
cursor
Bot
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a73a43adee
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| this.emit("error", error); | ||
| }); | ||
| if (this._eventsWiredClient) { | ||
| this._eventsWiredClient.removeListener("error", this._errorHandler); |
There was a problem hiding this comment.
When client is replaced while the previous Redis connection is still open or connecting, removing its only error listener leaves that connection active and unguarded. A later socket error from the old client is therefore emitted as an unhandled Node.js error event and can terminate the process; either close the previous connection before detaching it or retain an error handler until its lifecycle ends.
Useful? React with 👍 / 👎.
|
|
||
| redisClient.on('end', () => { | ||
| console.log('Redis client disconnected'); | ||
| `error`, `connect`, `disconnect`, and `reconnecting` are re-emitted on the adapter. Other Redis-client-only events (for example `ready`) can still be listened on `store.client`: |
There was a problem hiding this comment.
The newly documented disconnect subscription never fires for an actual node-redis disconnection: the Redis client signals connection termination with end (as the previous example in this README correctly used), while initClient() subscribes to a client event named disconnect. Users following this example will silently miss disconnect notifications, so the adapter should translate the client's end event or the documentation should not advertise this event.
Useful? React with 👍 / 👎.
| */ | ||
| export function createKeyvNonBlocking( | ||
| connect?: string | RedisClientOptions | RedisClientType, | ||
| connect?: KeyvRedisConnect, |
There was a problem hiding this comment.
Expanding createKeyvNonBlocking to accept KeyvRedisConnect advertises cluster and sentinel support, but the implementation still casts every connection to RedisClientType and mutates standalone-only options.disableOfflineQueue and options.socket. Cluster and sentinel node settings live in their respective nested client defaults, so these assignments do not disable their offline queues or reconnect strategies; when Redis is unavailable, the supposedly non-blocking secondary cache can therefore continue queueing or retrying and delay requests.
Useful? React with 👍 / 👎.
2 participants
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix, refactor, docs, and tests from a v6 readiness pass on
@keyv/redis.The adapter already implements the v6 storage contract (
capabilities.expires, absolutePXATwithPXfallback, cluster/sentinel). This PR closes the remaining gaps:Fixes
clientleaked listeners on the previous connection and kept a stale PXAT capability cache. Listeners are removed and PXAT is re-detected.getManyandclearswallowedthrowOnConnectErrorfailures. They now shareshouldRethrow()with the other batch methods.connectionTimeoutleft a dangling timer after a successful connect.createKeyvduplicated Keyv wiring and did not accept cluster/sentinel connect options. It now uses a sharedKeyvRedisConnecttype.Adapter layout and jsDocs
@paramtypes and@returns.nullreplies fromget/getMany/iteratormap toundefined(nevernull).on/once/emit) and re-emiterror,connect,disconnect, andreconnectingfrom the client.Docs
createKeyv,createKeyvNonBlocking,defaultReconnectStrategy), and the Hookified event table.useKeyPrefix, relativettlon adapterset,createClient(url),PX 0,keyv.useUnlink) are gone.Tests
test()withshould …descriptions throughout.undefinedandnot.toBeNull().once, client re-emit, no duplicate listeners, listener removal on client swap, PXAT reset.Testing
Standalone Redis 7 (
redis-serveron localhost:6379): 12 files / 175 tests passed. Cluster and sentinel suites were not run in this environment (no Docker). Coverage on standalone is ~83% because cluster-only branches are unexercised; GitHub Actions should cover those.Left as non-blocking (not in this PR):
iterator()still throws regardless ofthrowOnErrors(reasonable for generators); RedisSCAN MATCHtreats*in a namespace as a glob;@redis/clientkeyPrefixstill should not be combined with adapter namespaces.