Skip to content

fix(client): make unstable cmds throw #2990

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

Merged
merged 1 commit into from
Jun 6, 2025

Conversation

nkaradzhov
Copy link
Collaborator

As per the docs, unstableResp3 commands should throw when client is created with { RESP: 3, unstableResp3: false|undefined }

fixes #2989


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)?

As per the docs, unstableResp3 commands should throw
when client is created with { RESP: 3, unstableResp3: false|undefined }

fixes redis#2989
@@ -38,7 +38,11 @@ export function attachConfig<
Class: any = class extends BaseClass {};

for (const [name, command] of Object.entries(commands)) {
Class.prototype[name] = createCommand(command, RESP);
if (config?.RESP == 3 && command.unstableResp3 && !config.unstableResp3) {
Copy link
Member

Choose a reason for hiding this comment

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

@nkaradzhov I just remembered that I saw this piece of code which was supposed to attach a command that throws an error. Maybe we can fix that code instead.

Copy link
Collaborator Author

@nkaradzhov nkaradzhov Jun 5, 2025

Choose a reason for hiding this comment

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

@bobymicroby this is working properly. The problem is that it is executed only for module commands. And we have XREAD and XREADGROUP as part of the main client. So we should add the same logic in the place we attach the core functions as well

Copy link
Member

Choose a reason for hiding this comment

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

got it

Copy link
Member

@bobymicroby bobymicroby left a comment

Choose a reason for hiding this comment

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

lgtm

@nkaradzhov nkaradzhov merged commit 62ac8b7 into redis:master Jun 6, 2025
11 checks passed
@nkaradzhov nkaradzhov deleted the unstable-commands-should-throw branch June 6, 2025 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unstable core commands dont throw without unstableResp3
2 participants