Skip to content

Add support for client reply command #2503

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adoeppner
Copy link

Description

Add support for client reply command. Fixes #1913.


Checklist

  • Does npm test pass with this change (including linting)?
  • Is the new or changed code fully tested?
  • Is a documentation update included (if this change modifies existing APIs, or introduces new ones)?

@leibale
Copy link
Contributor

leibale commented May 11, 2023

Thanks for contributing! But... this command is a little bit tricky, because after CLIENT REPLY OFF the behavior of the client changes:

  1. The command promise should be resolved as soon as the command is written to the socket (as there is no other way to know if it was executed or not).
  2. The types should change (all the commands should change to Promise<void>).

The "Good First Issue" label on the issue was misleading, so I removed it now... Sorry about that, and thanks again for contributing! :)

@adoeppner
Copy link
Author

Thanks for contributing! But... this command is a little bit tricky, because after CLIENT REPLY OFF the behavior of the client changes:

  1. The command promise should be resolved as soon as the command is written to the socket (as there is no other way to know if it was executed or not).
  2. The types should change (all the commands should change to Promise<void>).

The "Good First Issue" label on the issue was misleading, so I removed it now... Sorry about that, and thanks again for contributing! :)

You're right. I'll look later into it and try to implement a solution

@leibale leibale marked this pull request as draft May 24, 2023 10:40
@leibale
Copy link
Contributor

leibale commented May 24, 2023

@adoeppner I've converted this PR to a draft PR, please mark it "ready for review" when you finish working on it :)

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.

Add support for redis command: CLIENT REPLY
2 participants