createSentinel with clientSideCache fail to connect() due to RESP version

[BrianMwit]

Description

When creating Sentinel with clientSideCache as follows

const client = await createSentinel({
    name: 'mycache',
    sentinelRootNodes: [{ host: '10.1.2.3', port: 12345 }],
    RESP: 3,
    clientSideCache: { ttl: 0, maxEntries: 0, evictPolicy: 'LRU'},
})
    .on('error', (err) => console.log("Redis Client Error", err))
    .connect();

This is based on configuration of this test:

node-redis/packages/client/lib/sentinel/index.spec.ts

Lines 48 to 54 in 6f3380b

	it('should not throw when clientSideCache is enabled with RESP 3', () => {
	assert.doesNotThrow(() =>
	RedisSentinel.create({
	...options,
	clientSideCache: clientSideCacheConfig,
	RESP: 3 as const,
	})

As per the test, createSentinel() does not throw
However, connect() error with "Client Side Caching is only supported with RESP3"

We found that this is likely because while clientSideCache option is passed from Sentinel option through to nodeClientOption, RESP setting is not.
This results in Sentinel communication using RESP 3 while node communication using RESP 2.

node-redis/packages/client/lib/sentinel/index.ts

Lines 683 to 694 in 6f3380b

	this.#nodeClientOptions = (options.nodeClientOptions ? {...options.nodeClientOptions} : {}) as RedisClientOptions<M, F, S, RESP, TYPE_MAPPING, RedisTcpSocketOptions>;
	if (this.#nodeClientOptions.url !== undefined) {
	throw new Error("invalid nodeClientOptions for Sentinel");
	}
	if (options.clientSideCache) {
	if (options.clientSideCache instanceof PooledClientSideCacheProvider) {
	this.#clientSideCache = this.#nodeClientOptions.clientSideCache = options.clientSideCache;
	} else {
	const cscConfig = options.clientSideCache;
	this.#clientSideCache = this.#nodeClientOptions.clientSideCache = new BasicPooledClientSideCache(cscConfig);
	// this.#clientSideCache = this.#nodeClientOptions.clientSideCache = new PooledNoRedirectClientSideCache(cscConfig);

Adding nodeClientOptions: { RESP: 3 }, to sentinel option fixed the issue

We belive the intended behaviour is to have node client use the same RESP version as the sentinel. Type declaration for the node client inherits RESP from the sentinel itself.

node-redis/packages/client/lib/sentinel/index.ts

Lines 604 to 624 in 6f3380b

	class RedisSentinelInternal<
	M extends RedisModules,
	F extends RedisFunctions,
	S extends RedisScripts,
	RESP extends RespVersions,
	TYPE_MAPPING extends TypeMapping
	> extends EventEmitter {
	#isOpen = false;
	get isOpen() {
	return this.#isOpen;
	}
	#isReady = false;
	get isReady() {
	return this.#isReady;
	}
	readonly #name: string;
	readonly #nodeClientOptions: RedisClientOptions<M, F, S, RESP, TYPE_MAPPING, RedisTcpSocketOptions>;

Please consider automatically passing RESP option from Sentinel client through to the node and/or updating the test to include integration between RedisSentinel and RedisClient

Node.js Version

22.16.0

Redis Server Version

6.0

Node Redis Version

5.5.6

Platform

Linux

Logs

Error: Client Side Caching is only supported with RESP3
    at #validateOptions (/app/node_modules/@redis/client/dist/lib/client/index.js:196:19)
    at new RedisClient (/app/node_modules/@redis/client/dist/lib/client/index.js:179:30)
    at new Class (/app/node_modules/@redis/client/dist/lib/commander.js:9:45)
    at /app/node_modules/@redis/client/dist/lib/client/index.js:80:34
    at RedisClient.create (/app/node_modules/@redis/client/dist/lib/client/index.js:84:35)
    at #createClient (/app/node_modules/@redis/client/dist/lib/sentinel/index.js:444:33)
    at RedisSentinelInternal.transform (/app/node_modules/@redis/client/dist/lib/sentinel/index.js:866:50)
    at #connect (/app/node_modules/@redis/client/dist/lib/sentinel/index.js:517:28)
    at process.processTicksAndRejections (node:internal/process/task_queues:105:5)
    at async RedisSentinelInternal.connect (/app/node_modules/@redis/client/dist/lib/sentinel/index.js:496:13)
    at async Class.connect (/app/node_modules/@redis/client/dist/lib/sentinel/index.js:236:9)

[nkaradzhov]

@BrianMwit thanks, we are gonn look into it