socketio / socket.io-redis-streams-adapter

The Socket.IO adapter based on Redis Streams, allowing to broadcast events between several Socket.IO servers.
https://socket.io/docs/v4/redis-streams-adapter/
MIT License
30 stars 15 forks source link

Request support for ioredis #2

Closed seolhw closed 8 months ago

MGTechTeam1 commented 1 year ago

Bump +1

titk12 commented 1 year ago

Bump +1

rajshrimohanks commented 1 year ago

This is much needed. Right now, the only reason why this package depends on node-redis is because the code uses the commandOptions method to set the .xread() method to run in a new Redis connection. This is node-redis specific syntax. Just making this generic would allow ioredis also to be used.

1finedev commented 1 year ago

Bump +1

troyflinn commented 1 year ago

Also like to see ioredis support for high availability application deployment utilising redis in sentinel mode. Sentinel not currently supported by node-redis, only ioredis.

cody-evaluate commented 1 year ago

Bump +1 for ioredis support

artyomtrubchik commented 10 months ago

My situation is that I have to use ioredis. +1

cyxn commented 9 months ago

+1

darrachequesne commented 8 months ago

Support for ioredis has been added in https://github.com/socketio/socket.io-redis-streams-adapter/commit/58faa1d9c8cf87282b8643ed0c1aa79322b81852, included in release 0.2.0.

jwetzell commented 8 months ago

@darrachequesne With the dependency on the commandOptions function from redis am I right that it is still required that projects that are using this library include redis as a dependency whether they are using redis or ioredis? If so why is redis not an actual dependency of this library and not just a dev dependency?

darrachequesne commented 7 months ago

@jwetzell that's a good question. Not sure how we can get rid of this import. Any idea?

Reference: https://github.com/redis/node-redis/blob/dbf8f59a47573e6a1c75b78e566af8c493015d5d/packages/client/lib/command-options.ts#L7-L10

jwetzell commented 7 months ago

@darrachequesne looks like the commandOptions function is exposed on the redisClient itself as well. Should just be able to call redisClient.commandOptions({isolated:true}) which I quickly tested and both ioredis and redis tests pass. It does seem like it is necessary as removing it does make the redis package not happy.

darrachequesne commented 7 months ago

@jwetzell nice! Does it work with a Redis cluster too?

jwetzell commented 7 months ago

@darrachequesne 😞 it does not... that function doesn't seem to be exposed on the Cluster class from node-redis. That is real annoying.

thiagon commented 7 months ago

At least redis should be as a peerDependencies, like socket.io-adapter

But is it really necessary to have a dependency just for this function? https://github.com/redis/node-redis/blob/dbf8f59a47573e6a1c75b78e566af8c493015d5d/packages/client/lib/command-options.ts#L7-L10

export function commandOptions<T>(options: T): CommandOptions<T> {
    (options as any)[symbol] = true;
    return options as CommandOptions<T>;
}
jwetzell commented 7 months ago

At least redis should be as a peerDependencies, like socket.io-adapter

But is it really necessary to have a dependency just for this function? https://github.com/redis/node-redis/blob/dbf8f59a47573e6a1c75b78e566af8c493015d5d/packages/client/lib/command-options.ts#L7-L10

export function commandOptions<T>(options: T): CommandOptions<T> {
    (options as any)[symbol] = true;
    return options as CommandOptions<T>;
}

As far as I can tell for node-redis the first argument needs to pass the isCommandOptions test located in the same file as that function you linked. The first line in the commandOptions function is the deal breaker. The use of the symbol primitive as the key means we can't just create an object that would pass that test without that symbol.