Node: Enable +/-inf as score for ZADD

[jamesx-improving]

Issue link

This Pull Request is linked to issue (URL): #3360

Checklist

Before submitting the PR make sure the following are checked:

• This Pull Request is related to one issue.
• Commit message has a detailed description of what changed and why.
• Tests are added or updated.
• CHANGELOG.md and documentation files are updated.
• Destination branch is correct - main or release
• Create merge commit if merging release branch into main, squash otherwise.

[Yury-Fridlyand]

Update zadd in transaction too
Add changelog

[Yury-Fridlyand]

Suggested change

| Record<string, number>

I think you can remove this line, because InfScore already includes a number.

[Yury-Fridlyand]

Thinking further I think you can update SortedSetDataType

export type SortedSetDataType = {
    /** The sorted set element name. */
    element: GlideString;
    /** The element score. */
    score: InfScore;
}[];

[jamesx-improving]

SortedSetDataType is used as the return type of many sorted set commands. Changing it will be too much of an impact, not to mention we are avoiding a "breaking change" here.

Same concern resulting me not removing | Record<string, number>, but I'm not entirely sure if removing it would count as a "breaking change". Please advise.

[Yury-Fridlyand]

Oh right.
Before suggesting other destructive ideas, could you please try ZADD with Number.POSITIVE_INFINITY and Number.NEGATIVE_INFINITY?
Probably we just should add extra handling and convert those to +-inf? No type changes needed at all in that case.

[jamesx-improving]

Number.POSITIVE_INFINITY and Number.NEGATIVE_INFINITY works with existing API, no change needed. Have suggested the same as workaround, but I don't think we can use that as a reason to not do this fix.

[jamesx-improving]

If the standard of non-breaking change is that existing downstream code still works, I think we can go ahead and modify both the object type (SortedSetDataType -> ElementAndScore) as well as the Record type (Record<string, number> -> Record<string, Score>). User will be able to feed +inf and -inf using either of the formats. I've tested and all existing tests passed after the change. Will update the PR for review.

[avifenesh]

Well, I'm not sure what the standard is, i can check it.
To my understanding it means breaking. Like literally breaking.

If nothing is going to happen to the user code, and he can keep running the same code, it's not breaking anything.

I'll ask Google to make sure.

[avifenesh]

[avifenesh]

I think we are good.

[jamesx-improving]

Great. Have updated the PR accordingly. Please review again 🙏

[jamesx-improving]

@avifenesh I followed how SortedSetDataType is defined as an array, instead of your approach of adding | ElementAndScore[] towards the end. I understand that | ElementAndScore[] gives user more flexibility, as they could feed either an array or a single object. However, I feel like this is deviated from the main purpose of this PR.

Please comment.

[avifenesh]

Does ElementAndScore is always an array?
I find it very limiting.
I would prefer

export type ElementAndScore = {
    /** The sorted set element name. */
    element: GlideString;
    /** The element score. */
    score: Score;
};

And

     public async zadd(
        key: GlideString,
        membersAndScores: ElementAndScore | ElementAndScore[] | Record<string, Score>,
        options?: ZAddOptions,
    ):

[avifenesh]

Or does the non Array version is not relevant?
If so,

public async zadd( key: GlideString, membersAndScores: ElementAndScore[] | Record<string, Score>, options?: ZAddOptions, ):

[avifenesh]

And if still array so at least ElementsAndScores since it is not a singular. But I would really prefer an array of custom type, then costume type which is an array of unnamed object, with another type in it.
Array is a real type, unnamed object is the one I would give the customization to, and it will be more dynamic.

[jamesx-improving]

I totally agree that giving a name to an object makes more sense than to an array of it. Also, this is another example of using the same type on multiple commands, especially when it's an input-output crossover, being very problematic, as the output (return value) will always be an array, but not necessarily the input.

My actual concern on accepting ElementAndScore as a single object is, under corner cases, it creates ambiguity, as we don't know if user is passing a single ElementAndScore object, or a Record<string, Score>. Consider below example:

const ambiguousMembersAndScores = {
    element: "+inf",
    score: 42,
};

This could be interpreted as:

1. A single member-score pair, with "+inf" being the member, and 42 being the score.
2. Two member-score pairs, with the first member being "element", first score being +inf, and the second member being "score", and second score being 42.

Maybe we can get away with this ambiguity with some cleaver if-else condition in createZAdd(), but I'm not sure if it's worth it.

If you agree with me on this, I'll just change the definition of ElementAndScore to non-array, single object, and use ElementAndScore[] | Record<string, Score> as the type of membersAndScores on ZADD, so for both types in this union type, it is a one-to-one substitution (SortedSetDataType -> ElementAndScore[], Record<string, number> -> Record<string, Score>), potentially easier for user to understand, as we are pulling off this type change on a minor version (1.4), and trying not to make it a breaking change.

[avifenesh]

I agree with the last. Our goal is to remove Record<string, Score> later, right?
You can mark it as @dperecated in the docs above, so users will see. Never tried it, so just if it is not extremely complicated. Otherwise, add a note in which users should use the ElementAndScore and avoid Record<string, Score> as it going to be deprecated.