Ensure proxies respect command options#3261
Conversation
|
@watersRand thanks, I will have a look! |
nkaradzhov
left a comment
There was a problem hiding this comment.
There are more places where this._self._commandOptions or this._self.commandOptions is used:
node-redis/packages/client/lib/client/index.ts
Line 1186 in e4cc77e
node-redis/packages/client/lib/client/pool.ts
Line 145 in e4cc77e
node-redis/packages/client/lib/client/pool.ts
Line 158 in e4cc77e
It would be great if we can fix and test those as well.
| }); | ||
|
|
||
|
|
||
| testUtils.testWithClient('Module TypeMapping Fix', async client => { |
There was a problem hiding this comment.
maybe change the title to: "proxies respect command options"
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit fa944a8. Configure here.
|
@watersRand thanks, i will have a look at this. |
nkaradzhov
left a comment
There was a problem hiding this comment.
A few things are still open before this can go in:
- RESP default regression (commander.ts). const RESP = config?.RESP ?? 2 should stay ?? DEFAULT_RESP (which is 3). The library default is RESP3, and this binds RESP2 transformReply handlers at class-generation time for any client created without an explicit RESP — even though the connection still negotiates RESP3. Bugbot re-flagged this on the latest commit. Please revert to DEFAULT_RESP (and keep the import).
- Sentinel getter recurses / duplicate member (sentinel/index.ts). The added
get _commandOptions() { return this.commandOptions; } - recurses — the existing commandOptions getter already returns this._commandOptions, so commandOptions → _commandOptions → commandOptions → … overflows the stack. It also collides with the existing private _commandOptions? field (duplicate identifier), in both RedisSentinelClient and RedisSentinel. Sentinel already stores options in #commandOptions with a public commandOptions getter, so it needs a different shape than the standalone client. Suggest having the sentinel command funcs read this._self.commandOptions (the existing public getter) rather than inventing _commandOptions, or wiring the namespace _self the way commander.ts does — but not a self-referential getter.
- Cluster withCommandOptions not updated (cluster/index.ts). Still proxy._commandOptions = options; (plain replace), unlike client/pool which now do Object.assign(Object.create(this._commandOptions ?? null), options). Since the cluster command paths now read this._commandOptions, chaining withCommandOptions drops parent options like timeout. Please make it consistent.
- Duplicate import (cluster/index.spec.ts). import { RESP_TYPES } from '../RESP/decoder' appears twice. Won't compile.
- Unrelated formatting churn. Most of the diff is reformatting (noop, if( → if (, reindenting the .on() chain and the _ejectSocket block, type-map indentation) in files this fix doesn't functionally touch. It buries the real change and invites conflicts — please strip it and keep only the functional edits. If formatting is genuinely off, a separate PR is the place for it.
Also minor: the title still says "ensure module commands respect proxy typeMapping" — "proxies respect command options" matches the actual scope better now.
Once 1–4 are sorted and the formatting is reverted, happy to re-review. npm test (with linting) should catch 2 and 4 locally.
Thanks for the review! I acknowledge all the points and have updated the PR accordingly. The regressions, sentinel stack overflow, cluster inheritance logic, and duplicate imports are now fixed, and I've completely stripped out the unrelated formatting noise. |
fa944a8 to
c2de21e
Compare

Description
Closes #3055 ,a bug where commands generated via static factories (Modules and Functions) were incorrectly referencing the root client's options (
this._self._commandOptions) instead of the immediate instance's options (this._commandOptions).The Problem:
When using
.withCommandOptions()or.withTypeMapping(), the library creates a proxy object. However, because Modules and Functions were hard-coded to look at the internal_selfreference for their configuration, they completely ignored any user-defined type mappings or timeouts set on the proxy. This effectively broke RESP3 type mapping for all Redis Module commands.The Solution:
#createModuleCommandand#createFunctionCommandto referencethis._commandOptions.sendCommandto prioritize instance-level options.NamespaceProxyClienttype definition to include_commandOptionsto ensure type safety.Allows developers to seamlessly switch between different configurations—such as string vs. binary (Buffer) outputs—across all available commands, including built-in modules, third-party modules, and user-defined libraries (Redis Functions). It enables this flexibility on the fly without the overhead of re-initializing the entire connection or maintaining multiple client instances for different return types.
Checklist
npm testpass with this change (including linting)?Note
Medium Risk
Changes core command-option merging and dispatch for all client types; regressions could affect timeouts, abort signals, RESP type mapping, and cluster redirects, though coverage is expanded with new tests.
Overview
Fixes #3055:
withCommandOptions/withTypeMappingproxies were ignored for module, function, and some dispatch paths because handlers read the root instance’s_commandOptionsinstead of the proxy’s.Command options are now layered with a prototype chain (
Object.assign(Object.create(...), overrides)) instead of shallow spreads, so nested proxies inherit base settings (e.g.timeout) while overrides (e.g.typeMapping) stay on the leaf. Merging atsendCommandand poolsendCommanduses the same pattern so inherited keys are not dropped.duplicate()on client and cluster flattens that chain via#flattenCommandOptionsso new instances get a plain mergedcommandOptionsobject. Module namespaces expose_commandOptionsthrough a getter tied to the current receiver (commander.ts).The same behavior is applied across standalone client, pool, cluster (including per-redirect execution client wiring), and sentinel command factories. New integration tests cover proxy inheritance, nested chains, and
duplicate().Reviewed by Cursor Bugbot for commit c2de21e. Bugbot is set up for automated code reviews on this repo. Configure here.