[prototype, for discussion] Add a node-redis backed connection alongside ioredis - #79
Draft
DavideCarvalho wants to merge 1 commit into
Draft
Conversation
Adds a "NodeRedisConnection" alongside the existing ioredis connections, opted into per connection using "client: 'node-redis'" in the redis config file. The ioredis code path is left untouched. node-redis requires an awaited "connect()" call, which cannot happen inside a constructor resolved synchronously by the IoC container. The connection object is therefore a synchronously created handle, and the client creation plus the "connect()" call live inside a single memoized promise that every command awaits before running. This is a prototype for the discussion in adonisjs#78, not a merge ready feature. Cluster, pub/sub subscriptions, health checks, tracing channels and the complete command surface are deliberately not implemented.
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
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.
Refs #78 — "Support node-redis alongside ioredis, now that ioredis is in maintenance mode". That issue ended with "happy to prototype if there's interest"; this is that prototype, built so you have something real to react to instead of a paragraph of speculation.
What this is
A
NodeRedisConnectionthat lives next to the ioredis connections, opted into per connection inconfig/redis.ts. The ioredis path is not modified: no changes toAbstractConnection,RedisConnection,RedisClusterConnectionorio_methods.ts, and the existing test suite passes unchanged.The async connect problem, and the answer I picked
This is the part worth your attention. node-redis needs an awaited
connect(), butredis.connection()resolves synchronously inside the container and constructors cannot be async. That is what stalled the first attempt on the bus side (elee1766 on boringnode/bus#41); niksy's boringnode/bus#61 works around it with lazily created connection promises.The approach here is the same idea adapted to this package's lifecycle:
The connection object is a synchronously created handle. The client creation and the
connect()call live inside a single memoized promise, and every command awaits that promise before running.Why this and not the alternatives:
boot()? It would make every app pay a Redis round-trip at boot for connections it may never use, and it would break the lazy-connection semantics ioredis connections have today (#75explicitly went the other way: don't dial the server for a connection nobody used). The memoized promise keeps "never used, never dialled" true —quit()on a connection that never ran a command closes nothing and just emitsend.redis.connection()? That is a breaking change to the single most used method in the package.if (!client.isOpen) await client.connect()? node-redis throwsSocket already openedon a secondconnect(), and N concurrent commands issued before the first connect resolves would race. The memoized promise makes the first command pay the cost and every later one hit a resolved promise. There is a test for exactly this (three concurrentINCRs on a cold connection return 1, 2, 3).redisis an optional peer dependency, so it must never be loaded for apps that only use ioredis.await import('redis')inside the connect promise means a build of this package contains exactly one reference toredisin the emitted JS, and it is that dynamic import. The cost of that choice:.nodeConnectionisundefineduntil the connection is established.await connection.connect()returns the client, so nothing is unreachable, it just isn't synchronously available the way.ioConnectionis. If you'd rather haveredisas a hard dependency and a synchronously available client handle, that's a one-line change and I'd like to know.The config surface a user writes
client: 'node-redis'is the discriminant;GetConnectionTypepicks the class from it, soredis.connection('modern')is typed asNodeRedisConnectionandredis.connection('main')is stillRedisConnection. There's a type test for that.Implemented
connect(),quit(),disconnect(), "never dialled → nothing to quit"statustracked by hand in the ioredis vocabulary (wait/connecting/connect/ready/reconnecting/close/end) plusisReady(),isConnecting(),isClosed(),lastError— so code duck-typing onconnection.statusbehaves the same with either clientconnect,ready,error,reconnecting,end. That is what keepsRedisManagerworking unmodified — its error reporter and its "delete fromactiveConnectionsonend" bookkeeping just workredis.connection(), connection re-use,quit/quitAll/disconnect/disconnectAllset(k, v, { PX })and camelCase, not an ioredis emulation layerpublish()(both the promise and the callback overload)defineCommand()/runCommand(), including manager-level scripts registered before the connection existsRuntimeExceptionwith an actionable message whenredisisn't installedDeliberately NOT implemented
Listing these rather than half-doing them:
createCluster, and it would be a second class. Not attempted.subscribe/psubscribe/unsubscribe/punsubscribeexist but throw a clear "not implemented in this prototype" error — they have to exist forRedisManagerto typecheck, and I'd rather they fail legibly than asundefined is not a function.publishdoes work.RedisCheckandRedisMemoryUsageChecktakeConnection(the ioredis union) and reach into.ioConnection. I did not widen that union, because it would break those classes. A node-redis connection can't be passed to them today.redisCommandpublishes{ command: ioredis.Command }. node-redis'ssendCommandtakes a plain argument array, so wiring it up means deciding what the channel payload should look like for a non-ioredis client. That's a design question, not a coding one.await connection.connect().createClientPool), and thescripts/functionsoptions node-redis takes at client creation.configure/stub changes. The generatedconfig/redis.tsstill only mentions ioredis.API differences I actually hit
set(k, v, 'PX', 1000)becomesset(k, v, { PX: 1000 });hset→hSet,pttl→pTTL,setnx→setNX. There is no compatibility alias in node-redis. So a "client-agnostic surface" (issue question 3) can't be a thin rename — it would have to be a real translation layer, and I don't think a prototype should smuggle one in.connect()doesn't reject while the server is unreachable. With the default reconnect strategy, node-redis retries forever and theconnect()promise never settles — so a command awaiting it hangs, where ioredis fails it viamaxRetriesPerRequest. Users needsocket.reconnectStrategy/connectTimeoutto get a failure. This is the sharpest behavioural difference for AdonisJS apps and I think it deserves a documented default rather than being left to each app.closeevent, andreconnectingcarries no delay. I emitreconnectingwithwaitTime: 0to fit the existingConnectionEventsshape, and never emitclose/wait.node:*andsubscriber:*are never emitted either.quit()is deprecated in favour ofclose(),disconnect()in favour ofdestroy(), and both throwClientClosedErroron a client that was never opened — hence the explicit "never dialled" branch.defineCommandattaches the script to the client; node-redis takesscriptsat client creation only.runCommandthereforeEVALs the stored script, splitting args bynumberOfKeys.Public type impact
One widening, and one finding worth flagging:
RedisManager'sconnectionevent payload is nowRedisConnection | RedisClusterConnection | NodeRedisConnection. A listener that reaches for.ioConnectionwithout narrowing would need a check. I couldn't see a way to avoid it that doesn't lie about runtime (the alternative — casting at the emit site — silently hands you aRedisConnectionthat isn't one). Happy to make it generic over the app's declared connections instead if you'd prefer strict non-breakage; it costs a conditional type for the "no connections declared" case.RedisConnectionsList,GetConnectionTypeandRedisManagerFactory's constraint were widened (additive; existing configs resolve to exactly the same classes as before).Connection,.ioConnection,io_methods.tsand every existing export are untouched.redisas an optional peer +import typein the shipped.d.ts:build/src/types.d.tsandbuild/src/connections/node_redis_connection.d.tsreferenceredistypes. I checked what that means for an app that doesn't installredis: AdonisJS's own tsconfig setsskipLibCheck: true, so the unresolved import is silently ignored and those types degrade toanyrather than erroring (I verified both directions — withskipLibCheck: falseit's aTS2307). So it's safe in practice for AdonisJS apps, but it's a real constraint on the design and you may prefer hand-rolled structural types, or a hard dependency.Tests
Run against the same Redis the existing suite uses (
compose.yml/ thegrokzen/redis-clusterservice withSTANDALONE=trueon7007), no new provisioning:npm run lint,tsc --noEmitandnpm run compileall clean. The 23 new tests cover the lazy-connect behaviour, connect memoization under concurrency, connection failure and error propagation,quit/disconnect/post-close use, the command subset, the options-object form, lua scripts, and manager-level integration and type inference.What I'd like to know
awaitin userland?.ioConnectionvs.nodeConnection: parallel accessors (what this does), or is there appetite for a client-agnostic surface? Given difference [Feature Request] #1 above, I don't think the latter can be cheap.redis?Happy to rework, split, or close this.