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.

Sign up for GitHub

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

Jump to bottom

subscribe arguments are wrong #46

Closed
idris opened this issue Sep 10, 2019 · 3 comments · Fixed by #120
Closed

subscribe arguments are wrong #46

idris opened this issue Sep 10, 2019 · 3 comments · Fixed by #120
Labels
redis-doc Issue caused by https://github.com/antirez/redis-doc - would require changes in that repo before fix released

Comments

@idris
Copy link

idris commented Sep 10, 2019

subscribe in handy-redis is typed as subscribe(...channels: Array<[string]>): Promise<any>;, but it should be subscribe(...channels: Array<string>): Promise<any>;
@mmkal
Copy link
Owner

mmkal commented Sep 22, 2019 •
edited
Loading

I agree, Array<string> seems right to me, but the type comes from here: https://github.com/antirez/redis-doc/blob/1aa2882aecd31b410bc4cc0b09fbcf25700228ab/commands.json#L2487

For the type to be as you suggest, we'd need "type": "string" there rather than "type": ["string"] - so this seems like it's due to a bug in https://github.com/antirez/redis-doc.

In the meantime, arguments are flattened so calling subscribe(['foo']) may look weird but should work fine.

Let me know if you think the library should be handling ["string"] differently somehow.

mmkal added a commit to mmkal/redis-doc that referenced this issue Dec 24, 2019
@mmkal
fix: subscribe should be of type string
9b38a9c 
arguments for SUBSCRIBE were in the wrong format, which caused this bug: mmkal/handy-redis#46

this change makes them match unsubscribe: https://github.com/antirez/redis-doc/blob/c882f1d3a767a096453489b00d7ec5bcc2eceaa7/commands.json#L2997-L3010
@mmkal mmkal mentioned this issue Dec 24, 2019
Merged
@mmkal
Copy link
Owner

mmkal commented Dec 24, 2019 •
edited
Loading

@idris I opened a PR in redis-doc to fix this: redis/redis-doc#1229. For now, I'll keep this issue open since I would rather avoid maintaining manual overrides of the docs.

@mmkal mmkal added the redis-doc Issue caused by https://github.com/antirez/redis-doc - would require changes in that repo before fix label Dec 24, 2019
itamarhaber pushed a commit to redis/redis-doc that referenced this issue Dec 26, 2019
@mmkal @itamarhaber
fix: subscribe should be of type string (#1229)
6fb67f0 
arguments for SUBSCRIBE were in the wrong format, which caused this bug: mmkal/handy-redis#46

this change makes them match unsubscribe: https://github.com/antirez/redis-doc/blob/c882f1d3a767a096453489b00d7ec5bcc2eceaa7/commands.json#L2997-L3010
@mmkal mmkal mentioned this issue Jan 3, 2020
Merged
@mmkal mmkal closed this as completed in #120 Jan 4, 2020
mmkal added a commit that referenced this issue Jan 4, 2020
@mmkal
feat(redis): update redis commands (#120)
e298024 
this updates both the https://github.com/antirez/redis-doc and https://github.com/NodeRedis/redis-commands dependencies.

fixes #46

Mostly the changes are new commands, but some function can now be called like client.hset('myhash', ['field1', 'Hello']) rather than client.hset('myhash', 'field1', 'Hello'). This allows setting multiple fields, e.g. client.hset('myhash', ['field1', 'Hello'], ['field2', 'Goodbye']).

The last arg of setbit can also now be number, rather than a string. Likely the string form will be dropped in the next major version.

See the diff from the PR merging this change for examples in the snapshot tests.
@mmkal
Copy link
Owner

mmkal commented Jan 4, 2020

🎉 This issue has been resolved in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mmkal mmkal added the released label Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
redis-doc Issue caused by https://github.com/antirez/redis-doc - would require changes in that repo before fix released
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat(redis): update redis commands mmkal/handy-redis
2 participants
@idris @mmkal