ZADD does not account for null reply about node-redis HOT 10 OPEN

I think that's reasonable. Now we just need a name. But that's a problem for Future Leibale. And probably Future Guy. ;)

from node-redis.

@puchm

1. until we fix it you can use client.sendCommand to workaround it:

const reply = await client.sendCommand(['ZADD', 'key', 'NX', '1', 'member']);

2. there are 2 ways to fix it:
   1. changing ZADD type to always return number | null (which is not true in the "simple case" of ZADD).
   2. remove support for NX, XX, LT, and GT in the main ZADD command, keep the return type number, create ZADD_CONDITINAL (have a better name?) which will support all modifiers and have a return type of number | null.

Which one is better in your opinion? (@guyroyse @sjpotter)

from node-redis.

I'm leaning toward option 1. But I am concerned that either change could break existing code.

from node-redis.

Option 1 can be considered a bug fix, option 2 is a breaking change for sure. Should we do 1 for v4, and 2 for v5?

from node-redis.

I would oppose option 2 because I don't see a reason why ZADD should be treated any different from most other commands. This would need specific documentation just for ZADD. That is reasonable for things that are more complex like scan or transactions but not for ZADD in my opinion.

from node-redis.

@puchm the main reason is to simplify the "main case", i.e:

const reply = await client.zAdd('key', {
  member: 'a',
  score: 1
});

if NX, XX, LT, and GT are supported in the main ZADD, the type of reply will be number | null, which is "not true" because it'll never be a null unless you use one of NX XX LT or GT. By separating it into multiple commands we can have different return types for different cases (i.e. number for the "normal case", and number | null for the "conditional" case).

from node-redis.

there are 2 ways to fix it:

changing ZADD type to always return number | null (which is not true in the "simple case" of ZADD).
remove support for NX, XX, LT, and GT in the main ZADD command, keep the return type number, create ZADD_CONDITINAL (have a better name?) which will support all modifiers and have a return type of number | null.

Which one is better in your opinion? (@guyroyse @sjpotter)

I prefer the latter, as it makes the return type obvious. I dislike return types that are not possible, but now one has to code around them due to that.

from node-redis.

@leibale I guess the optimal way would be to use the power of Typescript to make the return type dynamic based on what is passed in, but I don't think that's possible with the current architecture. Still, the number of methods you have where the name is different from the actual Redis command is really low and should probably stay that way. I'm wondering if there are other cases where some return types are just not possible depending on what is passed in?

I.e. it looks to me that there is a similar situation with SET. If neither GET nor XX nor NX are given the return type of SET will never be null. How do you handle that?

from node-redis.

The "optimal way" would be to use TypeScript generics for that, but we are already "abusing" them for other things, which makes that very hard. This is the best option I found, but I hate it...

interface Command {
  transformArguments(...args: Array<unknown>): unknown;
}

function transformArguments(key: string, options?: { NUMBER: false; }): string;
function transformArguments(key: string, options?: { NUMBER: true; }): number;
function transformArguments(key: string, options?: { NUMBER: boolean; }) {
  if (options?.NUMBER) {
    return ['GET', key, 'NUMBER'] as never;
  }
  
  return ['GET', key] as never;
}

const GET = {
  transformArguments
} satisfies Command;

const COMMANDS = {
  GET,
  get: GET
};

type COMMANDS = typeof COMMANDS;

type RedisClient = {
  [COMMAND in keyof COMMANDS]: COMMANDS[COMMAND]['transformArguments'];
};

const client = undefined as unknown as RedisClient;

const a = client.get('key');
const b = client.get('key', { NUMBER: true });

if you have any ideas please share.. ;)

In SET we didn't do it since in the "normal case" the reply does not matter, but we did it in other commands (for example, XAUTOCLAIM_JUSTID).

from node-redis.

Having exactly this issue as well with the null reply screwing things up ...

from node-redis.