Skip to content

Support npm Node Redis v4 #337

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

Closed
wants to merge 4 commits into from
Closed

Conversation

BarryCarlyon
Copy link

This should fix #336

Makes connect redis compatible with node redis v4

TLDR:

  • fix all the redis calls to their upper case NAMES
  • fix all the redis calls to promises (then/catch version)
  • fix TTL to a string in the set function and call sendCommand instead of set as they have changed the args/array passing method too much
  • updated the readme to note the additional required explicit connect before passing the client to connect-redis

I did not revise/update the tests as yet. May do so if I get a moment

As a mimum should get people running as they migrate

Update redis to v4 in package.json
Update readme to include the new connect command
@wavded
Copy link
Collaborator

wavded commented Nov 29, 2021

Thanks @BarryCarlyon for tackling this! Ideally, we would not loose compatibility with node-redis: 3.2 and it must continue to work with ioredis and mockredis to not break other applications (have you tested any of that yet?). At first I thought perhaps we programmatically enable the legacyMode if we detect a v4 client (not sure if that is possible). @luin are these changes still compatible with ioredis?

@BarryCarlyon
Copy link
Author

@wavded haven't tested as I'm unfamiliar this those.

I'll see if I have some time to test with those rather than just node-redis

@BarryCarlyon
Copy link
Author

I had a poke about.

node-redis seems to have "broken" a lot of stuff when it comes to operating with "friendlyness" to other modules

For example how scan in node-redis no longer accepts an array of args.

So seems users can patch connect-redis to play well with node-redis, it won't remain interoperably with the family I don't think.

@wavded
Copy link
Collaborator

wavded commented Dec 13, 2021

I've come to a similar conclusion and am not recommending people upgrade to v4 at this time as v3 is still supported and seems faster. I have updated the readme and am watching the issues below to see if an upgrade path will become clearer:

redis/node-redis#1765
redis/node-redis#1740

@BarryCarlyon
Copy link
Author

BarryCarlyon commented Dec 13, 2021

Yeah poking about further.

it would be "solvable" if both used the same "send raw command function"

But node-redis uses sendCommand and ioredis uses call

And ndoe-redis did something really weird with scan

Fixed the scan function as the format has changed.
Tests don't pass yet. tripping up on ttl/line 102
@BarryCarlyon
Copy link
Author

I pushed up my progress on getting tests to pass.
needs further work but I will likely unpr the pr
Since I'm gonna dowgrade to node-redis 3 till node-redis can sort itself out.

The move to 4 was painful and as noted in the linked issues their migration guide is incomplete to say the least

@github-actions
Copy link

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Jan 13, 2022
@wavded wavded removed the stale label Jan 13, 2022
@wavded
Copy link
Collaborator

wavded commented Feb 4, 2022

Support for legacyMode has matured enough such that it passes all the tests. I added support for it in the most recent version so we'll see what happens. It is interoperable with the other clients so will keep it as a legacyMode option only for now until the possibility of having one API or close to one API without legacyMode works for all clients.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

connect-redis not working with latest version of node-redis client (https://github.com/redis/node-redis)
2 participants