Node: convert Set commands return type to Set #1299

shohamazon · 2024-04-17T15:59:54Z

Issue #, if available:

Description of changes:
Currently, the way we convert the rust value returning from core to a js value, is by using napi.
Since napi doesn't support sets, commmands with a return type of set, returns an array in node.
This is breaking the GLIDE API, since similar commands in different wrappers return a completely different type.
Until napi will add support for sets, we will convert each of the command to a Set, in the wrapper.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Yury-Fridlyand · 2024-04-17T21:52:32Z

node/src/BaseClient.ts

+        const result = (await this.createWritePromise(
+            createSMembers(key),
+        )) as string[];
+        return new Set(result);


A breaking change for user API
I think we have to document it in changelog and/or release notes.

I documented that in the changelog

There is a good practice to highlight those as a breaking change and/or track these changes in another doc
Should be also highlighted in release notes

node/src/RedisClusterClient.ts

Yury-Fridlyand · 2024-04-17T21:55:08Z

node/tests/TestUtilities.ts

@@ -143,7 +143,7 @@ export async function transactionTest(
    baseTransaction.sismember(key7, "bar");
    args.push(true);
    baseTransaction.smembers(key7);
-    args.push(["bar"]);
+    args.push(new Set(["bar"]));


Add a test for spopcount

barshaul

Is there a different from the user perspective that some of the API defined with 'async' and some not, or defining the function with async is only a matter of wrapping the function with a promise?
@avifenesh

node/src/BaseClient.ts

Yury-Fridlyand · 2024-04-18T18:37:52Z

node/src/BaseClient.ts

@@ -1271,23 +1278,28 @@ export class BaseClient {
     *
     * @param key - The key of the set.
     * @param count - The count of the elements to pop from the set.
-     * @returns A list of popped elements will be returned depending on the set's length.
-     * If `key` does not exist, empty list will be returned.
+     * @returns A set containing the popped elements, depending on the set's length.


emphasize set

Yury-Fridlyand · 2024-04-18T18:39:08Z

node/src/RedisClient.ts

@@ -102,7 +102,23 @@ export class RedisClient extends BaseClient {
     *      If the transaction failed due to a WATCH command, `exec` will return `null`.
     */
    public exec(transaction: Transaction): Promise<ReturnType[] | null> {
-        return this.createWritePromise(transaction.commands);
+        return this.createWritePromise(transaction.commands).then(


This block is repeated in both clients - can you move it to a function?

barshaul · 2024-05-02T13:25:15Z

CHANGELOG.md

@@ -8,6 +8,7 @@
 * Node: Added ZRANGE command ([#1115](https://github.com/aws/glide-for-redis/pull/1115))
 * Python: Added RENAME command ([#1252](https://github.com/aws/glide-for-redis/pull/1252))
 * Python: Added APPEND command ([#1152](https://github.com/aws/glide-for-redis/pull/1152))
+* Node: When receiving SPOP with count, SMEMBERS convert result to Set ([#1299](https://github.com/aws/glide-for-redis/pull/1299))


It should be under breaking changes section

barshaul · 2024-05-02T13:29:50Z

node/src/RedisClient.ts

@@ -102,7 +102,23 @@ export class RedisClient extends BaseClient {
     *      If the transaction failed due to a WATCH command, `exec` will return `null`.
     */
    public exec(transaction: Transaction): Promise<ReturnType[] | null> {
-        return this.createWritePromise(transaction.commands);
+        return this.createWritePromise(transaction.commands).then(
+            (result: unknown) => {


why is it unknown? can't you do (result: ReturnType[] | null)?

barshaul · 2024-05-02T13:47:43Z

node/src/Transaction.ts

@@ -115,8 +115,16 @@ export class BaseTransaction<T extends BaseTransaction<T>> {
     * @internal
     */
    readonly commands: redis_request.Command[] = [];
+    readonly set_commands_indexes: number[] = [];


It should be clear that it is for internal usage. Please add a documentation and mark it with @internal too.

still missing documentation

weird, I remember adding it

note: nodejs naming convention is lowerCamelCase
setCommandsIndexes

node/src/Transaction.ts

barshaul

round

barshaul · 2024-05-05T11:26:22Z

CHANGELOG.md

 #### Fixes
 * Python: Fix typing error "‘type’ object is not subscriptable" ([#1203](https://github.com/aws/glide-for-redis/pull/1203))
 * Core: Fixed blocking commands to use the specified timeout from the command argument ([#1283](https://github.com/aws/glide-for-redis/pull/1283))

+### Breaking Change


Breaking Changes

barshaul · 2024-05-05T11:29:09Z

CHANGELOG.md

 #### Fixes
 * Python: Fix typing error "‘type’ object is not subscriptable" ([#1203](https://github.com/aws/glide-for-redis/pull/1203))
 * Core: Fixed blocking commands to use the specified timeout from the command argument ([#1283](https://github.com/aws/glide-for-redis/pull/1283))

+### Breaking Change
+* Node: When receiving SPOP with count, SMEMBERS convert result to Set ([#1299](https://github.com/aws/glide-for-redis/pull/1299))


Changed smembers and spopCount functions to return Set instead of <?>

barshaul · 2024-05-05T13:52:51Z

node/src/BaseClient.ts

+    protected processResultWithSetCommands<ReturnType>(
+        result: ReturnType[] | null,
+        setCommandsIndexes: number[],
+    ): (ReturnType | Set<string>)[] | null {


add Set to ReturnType

avifenesh · 2024-05-06T08:28:33Z

Is there a different from the user perspective that some of the API defined with 'async' and some not, or defining the function with async is only a matter of wrapping the function with a promise? @avifenesh

@barshaul Please explain your question, not sure i understood.
A promise is wrapper for callback, async is a wrapper for promise. kind of.
If you return promise, from the user perspective it doesn't matter if it by defining a function as async or by explicitly returning a promise. For the end user its the same. If this is what you meant.
But its better to keep it consistent since it easier to understand as a reader.

avifenesh · 2024-05-06T08:34:50Z

Im sorry if im too late and i dont know if its actually possible, but why we dont try to instead of changing to set in the commands themself, to hook them on the way back from the core and convert the response to set base on the command?
It could be less changes, easier to add all command that are sets commands to this list already, without keep tracking the implementation of each in the future, and easier to change when napi will start giving support to sets.

shohamazon · 2024-05-06T10:39:35Z

Im sorry if im too late and i dont know if its actually possible, but why we dont try to instead of changing to set in the commands themself, to hook them on the way back from the core and convert the response to set base on the command? It could be less changes, easier to add all command that are sets commands to this list already, without keep tracking the implementation of each in the future, and easier to change when napi will start giving support to sets.

I don't think I fully understand where you intend to add the conversion

barshaul · 2024-05-06T14:33:25Z

Im sorry if im too late and i dont know if its actually possible, but why we dont try to instead of changing to set in the commands themself, to hook them on the way back from the core and convert the response to set base on the command? It could be less changes, easier to add all command that are sets commands to this list already, without keep tracking the implementation of each in the future, and easier to change when napi will start giving support to sets.

I don't think I fully understand where you intend to add the conversion

there isn't an option with napi to convert to nodejs set type.

Is there a different from the user perspective that some of the API defined with 'async' and some not, or defining the function with async is only a matter of wrapping the function with a promise? @avifenesh

@barshaul Please explain your question, not sure i understood. A promise is wrapper for callback, async is a wrapper for promise. kind of. If you return promise, from the user perspective it doesn't matter if it by defining a function as async or by explicitly returning a promise. For the end user its the same. If this is what you meant. But its better to keep it consistent since it easier to understand as a reader.

nvm, it was on outdated code

barshaul · 2024-05-06T14:40:19Z

Im sorry if im too late and i dont know if its actually possible, but why we dont try to instead of changing to set in the commands themself, to hook them on the way back from the core and convert the response to set base on the command? It could be less changes, easier to add all command that are sets commands to this list already, without keep tracking the implementation of each in the future, and easier to change when napi will start giving support to sets.

I'm unsure where exactly do you mean to add this hook, but when the nodejs wrapper receives a response from the core, it only contains the response value, not the command name.

Yury-Fridlyand · 2024-05-06T16:37:16Z

node/src/BaseClient.ts

+            result as ReturnType[];
+
+        for (const index of setCommandsIndexes) {
+            modifiedResult[index] = new Set(modifiedResult[index] as string[]);


just to confirm that there is no commands that return a Set of not-a-string

Yury-Fridlyand · 2024-05-06T16:40:36Z

node/src/BaseClient.ts

+        return this.createWritePromise<string[]>(createSMembers(key)).then(
+            (result: string[]) => {
+                const resultSet: Set<string> = new Set<string>(result);
+                return resultSet;
+            },
+        );


make a function which converts result to set and call it in then.
BTW, can you refer the Set constructor here?
Something like

return this.createWritePromise<string[]>(createSMembers(key)).then(Set<string>::new);

Yury-Fridlyand · 2024-05-06T16:41:35Z

node/src/RedisClient.ts

@@ -102,7 +102,14 @@ export class RedisClient extends BaseClient {
     *      If the transaction failed due to a WATCH command, `exec` will return `null`.
     */
    public exec(transaction: Transaction): Promise<ReturnType[] | null> {
-        return this.createWritePromise(transaction.commands);
+        return this.createWritePromise<ReturnType[] | null>(


Should be there set as a return type too?

it is, I didnt see that before

node/src/RedisClusterClient.ts

barshaul · 2024-05-07T09:10:07Z

CHANGELOG.md

 #### Fixes
 * Python: Fix typing error "‘type’ object is not subscriptable" ([#1203](https://github.com/aws/glide-for-redis/pull/1203))
 * Core: Fixed blocking commands to use the specified timeout from the command argument ([#1283](https://github.com/aws/glide-for-redis/pull/1283))

+### Breaking Changes
+* Node: Changed SMEMBERS and SPOP with count functions to return Set instead of string [] ([#1299](https://github.com/aws/glide-for-redis/pull/1299))


string [] -> string[]
please use the functions' name and not the redis command name (SMEMBERS -> smembers, SPOP with count ->spopCount)

barshaul · 2024-05-07T09:14:18Z

node/src/Transaction.ts

+     */
+    protected addAndReturn(
+        command: redis_request.Command,
+        is_set: boolean = false,


is_set -> shouldConvertToSet (note: nodejs naming convention is lowerCamelCase)

barshaul · 2024-05-07T09:21:06Z

node/src/Transaction.ts

+        is_set: boolean = false,
+    ): T {
+        if (is_set) {
+            // If the command is marked as a Set command, its index within the transaction is recorded for conversion.


The command's index within the transaction is saved for later conversion of its response to a Set type

barshaul

see inline comments

shohamazon added the node Node.js wrapper label Apr 17, 2024

shohamazon requested a review from a team as a code owner April 17, 2024 15:59

shohamazon changed the title ~~Node: convert Set commands return type to sets~~ Node: convert Set commands return type to Set Apr 17, 2024

shohamazon force-pushed the node_sets branch 2 times, most recently from e466a07 to 036df4d Compare April 17, 2024 16:05

Yury-Fridlyand reviewed Apr 17, 2024

View reviewed changes

barshaul reviewed Apr 18, 2024

View reviewed changes

shohamazon force-pushed the node_sets branch from 507b620 to 5ba6312 Compare April 18, 2024 11:23

Yury-Fridlyand reviewed Apr 18, 2024

View reviewed changes

barshaul reviewed May 2, 2024

View reviewed changes

node/src/Transaction.ts Outdated Show resolved Hide resolved

barshaul reviewed May 2, 2024

View reviewed changes

node/src/Transaction.ts Outdated Show resolved Hide resolved

barshaul requested changes May 2, 2024

View reviewed changes

shohamazon added 2 commits May 2, 2024 15:25

Node: convert Set commands return type to sets

7d40414

remove async from function signature

51e5711

shohamazon force-pushed the node_sets branch from e4c9947 to 01849a9 Compare May 5, 2024 06:42

pr comments

d332283

shohamazon force-pushed the node_sets branch from 01849a9 to d332283 Compare May 5, 2024 06:46

barshaul reviewed May 5, 2024

View reviewed changes

Yury-Fridlyand reviewed May 6, 2024

View reviewed changes

PR suggestions

a3f715e

barshaul reviewed May 7, 2024

View reviewed changes

barshaul approved these changes May 7, 2024

View reviewed changes

PR suggestions

0f206a4

shohamazon merged commit 279cfc9 into valkey-io:main May 7, 2024
12 checks passed

shohamazon deleted the node_sets branch May 7, 2024 12:02

tjzhang-BQ pushed a commit to Bit-Quill/valkey-glide that referenced this pull request May 8, 2024

Node: convert Set commands return type to Set (valkey-io#1299)

9679ae8

Yury-Fridlyand mentioned this pull request May 30, 2024

Node: added SINTER command Bit-Quill/valkey-glide#327

Merged

cyip10 pushed a commit to Bit-Quill/valkey-glide that referenced this pull request Jun 24, 2024

Node: convert Set commands return type to Set (valkey-io#1299)

b00da22

Yury-Fridlyand mentioned this pull request Jul 10, 2024

Node: add SUNION command #1919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Node: convert Set commands return type to Set #1299

Node: convert Set commands return type to Set #1299

shohamazon commented Apr 17, 2024 •

edited

Loading

Yury-Fridlyand Apr 17, 2024

shohamazon Apr 17, 2024

Yury-Fridlyand Apr 18, 2024

Yury-Fridlyand Apr 17, 2024

barshaul left a comment

Yury-Fridlyand Apr 18, 2024

Yury-Fridlyand Apr 18, 2024

barshaul May 2, 2024

barshaul May 2, 2024

barshaul May 2, 2024 •

edited

Loading

barshaul May 5, 2024

shohamazon May 6, 2024

barshaul May 7, 2024

barshaul left a comment

barshaul May 5, 2024

barshaul May 5, 2024

barshaul May 5, 2024

avifenesh commented May 6, 2024

avifenesh commented May 6, 2024

shohamazon commented May 6, 2024

barshaul commented May 6, 2024

barshaul commented May 6, 2024

Yury-Fridlyand May 6, 2024

Yury-Fridlyand May 6, 2024

shohamazon May 7, 2024

Yury-Fridlyand May 6, 2024

shohamazon May 7, 2024 •

edited

Loading

barshaul May 7, 2024

barshaul May 7, 2024 •

edited

Loading

barshaul May 7, 2024

barshaul left a comment

Node: convert Set commands return type to Set #1299

Node: convert Set commands return type to Set #1299

Conversation

shohamazon commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul May 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Breaking Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avifenesh commented May 6, 2024

avifenesh commented May 6, 2024

shohamazon commented May 6, 2024

barshaul commented May 6, 2024

barshaul commented May 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shohamazon May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul May 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

barshaul left a comment

Choose a reason for hiding this comment

shohamazon commented Apr 17, 2024 •

edited

Loading

barshaul May 2, 2024 •

edited

Loading

shohamazon May 7, 2024 •

edited

Loading

barshaul May 7, 2024 •

edited

Loading