fix(caching-redis): read ioredis options from redisOptions for consistency with other Redis modules - #16101
Conversation
The connection loader destructured module options with a rest element and spread the remainder into the ioredis client. A nested redisOptions object was therefore passed through as a literal redisOptions key, which ioredis ignores, while the module's own ttl, prefix and compressionThreshold keys were forwarded to the client instead. Read redisOptions explicitly and keep the module options out of the client, matching cache-redis, locking-redis and event-bus-redis. Top-level ioredis options keep working and now log a deprecation warning, the same shim workflow-engine-redis uses for this migration. Fixes medusajs#16099
🦋 Changeset detectedLatest commit: 769090b The changes in this PR will be included in the next version bump. This PR includes changesets to release 79 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Thanks for the contribution! Initial automated review looks good. Well-scoped fix for #16099: caching-redis now reads ioredis options from a nested redisOptions object (matching cache-redis, locking-redis, event-bus-redis, workflow-engine-redis) and stops forwarding module-own options (ttl, prefix, compressionThreshold) to the client. The flat top-level shape is kept behind a @deprecated alias with a runtime deprecation warning, preserving backward compatibility. Destructuring does not affect the later prefix registration. Includes a changeset and 9 focused unit tests with ioredis mocked. No security or performance concerns. Triggered by: new PR opened |
Summary
What — What changes are introduced in this PR?
The
caching-redisprovider now reads its ioredis options from a nestedredisOptionsobject, the same shapecache-redis,locking-redisandevent-bus-redisalready use. The module's own options (ttl,prefix,compressionThreshold) are no longer forwarded to the Redis client. Top-level ioredis options continue to work and now log a deprecation warning. Fixes #16099.Why — Why are these changes relevant or necessary?
Configuring the Redis cache is currently a coin flip.
redisOptionsis the shape every other Redis module in the repo accepts, and it is the shapeworkflow-engine-redisexplicitly tells you to migrate to, but incaching-redisit is the one shape that does nothing.connection.tsdestructured with a rest element and spread the remainder into the client, soredisOptionsarrived at ioredis as a literalredisOptionskey that it ignores, and the setting was dropped without a word:The same line has a second effect in the other direction, as the triage bot noted on the issue:
ttl,prefixandcompressionThresholdare module options consumed elsewhere (connection.tsregistersprefix;redis-cache.tsreadscompressionThreshold), and they were being handed to a Redis client that has no use for them.The nested shape is not a preference here, it is settled convention in this repo.
workflow-engine-redisdeprecates its flatoptionskey with the words "UseredisOptionsinstead for consistency with other modules" and warns at runtime.caching-redisis the module that did not follow.How — How have these changes been implemented?
types/index.tsgainsredisOptions?: RedisOptions. The flat inheritance is kept behind a@deprecated-annotated alias so existing configs still typecheck.loaders/connection.tspulls the module's own keys out explicitly, prefers the nested object, falls back to the deprecated flat rest, and warns once when the fallback is used:This mirrors
workflow-engine-redis/src/loaders/redis.ts, which implementsnewRedisOptions ?? deprecatedRedisOptionsplus a warning for exactly this migration.@medusajs/caching-redisis published at 2.17.2, so dropping the flat path outright would break deployments already passing it. Happy to take the breaking change instead if you would rather, since the package is young.Testing — How have these changes been tested, or how can the reviewer test the feature?
New
src/loaders/__tests__/connection.spec.ts, 9 tests, ioredis mocked to capture the constructor arguments. No Redis server required.The tests pin the bug rather than restate the implementation. Reverting
connection.tsandtypes/index.tsto their current state ondevelopwhile keeping the new tests fails 5 of 9:redisOptionsobject to the ioredis clientkeepAliveisundefined(the reported bug)ttl: 60reaches ioredisredisOptionsoverride a defaultmedusa-cache-redis, wantedcustom-connectionredisOptionsover deprecated top-level options1, wanted10000The other 4 pass with and without the change, which is the point: they cover the invariants this PR preserves (built-in defaults kept, no warning on the
redisOptionspath,prefixstill registered,redisUrlstill required).Also run: full package suite 11/11 pass (the 9 new plus the existing
redis-cache.spec.ts),prettier --checkclean.tsc --buildreports 4TS2307 Cannot find module '@medusajs/framework/...'errors from unbuilt workspace deps, but the identical set appears on an untouched tree including in files this PR does not touch, so it is environmental and this change adds none.Examples
The previous flat form still works, and logs a deprecation warning pointing at
redisOptions:Checklist
Please ensure the following before requesting a review:
@medusajs/caching-redis: patch)Additional Context
Closes #16099. Reported by @thaind-taureau, who diagnosed the root cause and the exact lines; the triage bot independently confirmed both halves of the defect.
Prior art for the shape, all in this repo:
packages/modules/cache-redis/src/loaders/index.ts:const { redisUrl, redisOptions } = optionsthen...(redisOptions ?? {})packages/modules/providers/locking-redis/src/loaders/index.ts: samepackages/modules/event-bus-redis/src/loaders/index.ts: samepackages/modules/workflow-engine-redis/src/loaders/redis.ts:newRedisOptions ?? deprecatedRedisOptionsplus the deprecation warning this PR copies