Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add binary variant to PUBSUB subscribe commands #420

Conversation

yipin-chen
Copy link

@yipin-chen yipin-chen commented Sep 4, 2024

Add binary variant for the following commands:

  • PSubbscribe
  • Subscribe
  • SSubscribe
  • Unsubscribe
  • SUnsubscribe
  • PUnsubscribe

Add decoder option to PUBSUB's createClient().

Note: to complete PUBSUB subscribe commands, I have to cherry-pick already merged main branch PR valkey-io#2201.

* Node: add binary support for PubSub commands, part 1

---------

Signed-off-by: Yi-Pin Chen <[email protected]>
Copy link

@acarbonetto acarbonetto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

node/src/BaseClient.ts Outdated Show resolved Hide resolved
node/tests/PubSub.test.ts Outdated Show resolved Hide resolved
node/tests/PubSub.test.ts Outdated Show resolved Hide resolved
node/tests/PubSub.test.ts Outdated Show resolved Hide resolved
node/tests/PubSub.test.ts Outdated Show resolved Hide resolved
node/tests/PubSub.test.ts Outdated Show resolved Hide resolved
node/tests/PubSub.test.ts Outdated Show resolved Hide resolved
method,
listeningClient,
context,
index,
);
))!;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you add this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VS Code complaints that pubsubMsg is possibly null. So I have to add this.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't review tests, I trust you. If they past - all LGTM

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All PubSub tests passed.

@yipin-chen yipin-chen merged commit 04e9334 into node/integ-valkey-293-return-record-with-glidestring Sep 5, 2024
4 of 8 checks passed
@yipin-chen yipin-chen deleted the node/integ-yipin-subscribe-valkey-306 branch September 5, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants