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

Question: support for node-redis version 4 #330

Closed
xavizus opened this issue Sep 9, 2021 · 11 comments
Closed

Question: support for node-redis version 4 #330

xavizus opened this issue Sep 9, 2021 · 11 comments
Assignees
Labels

Comments

@xavizus
Copy link

xavizus commented Sep 9, 2021

Hi,
Is there any plans to add support for node-redis version 4?
There are some breaking changes when it comes to Typescript.

@wavded
Copy link
Collaborator

wavded commented Sep 9, 2021

Yes. It appears we can run this in legacyMode to maintain support without changing our implementation.

@wavded wavded self-assigned this Sep 9, 2021
@xavizus
Copy link
Author

xavizus commented Sep 9, 2021

Yes, that's correct. Though, with Typescript there are some more changes.
Instead of being the type RedisClient, you get RedisClientType<RedisModules, RedisLuaScripts>, while expected type is redis.RedisClient | ioRedis.Redis | ioRedis.Cluster;
It can be temporary solved, by adding <any> before the redisclient when you create the redis store.
For example
new redisStore( { client: <any>redisClient, ttl: 8 * 60 * 60 } ),

@wavded
Copy link
Collaborator

wavded commented Sep 9, 2021

I'm unsure how those Typescript changes affect this project as it doesn't use Typescript. Is this a matter of updating the @types/connect-redis to support? If so, a PR would need to be created to support the change over there:

https://www.npmjs.com/package/@types/connect-redis

@xavizus
Copy link
Author

xavizus commented Sep 9, 2021

I just tried the following:

Seems like it does not work with the new redis version anyways.

```javascript
import express, { Request, Response, NextFunction } from "express";
import connectRedis from "connect-redis";
import session from "express-session";
import * as redis from "redis";

( async() =>
{
   const app = express();
   const redisStore = connectRedis( session );
   const redisClient = redis.createClient( { legacyMode: true } );
   await redisClient.connect();
   app.use( session( {
      store: new redisStore( { client: <any>redisClient, ttl: 8 * 60 * 60 } ),
      secret: "mySecretPassword",
      saveUninitialized: false,
      resave: false,
      cookie: {
         sameSite: "strict",
         secure: false,
         httpOnly: true,
      }
   } ) );

   app.get( "/", ( request: Request, response: Response, _: NextFunction ) =>
   {
      request.session.views = 1;
      console.log( request.session );
      response.end();
   } );

   app.listen( 3000, async() =>
   {
      console.log( `${process.pid} is now listening!` );
   } );
} )();

And I get the following error when doing response.end()

TypeError [ERR_INVALID_ARG_TYPE]: The "string" argument must be of type string or an instance of Buffer or ArrayBuffer. Received an instance of Array
    at new NodeError (node:internal/errors:371:5)
    at Function.byteLength (node:buffer:734:11)
    at encodeCommand (/home/stephan/Documents/DEV/backend/node_modules/redis/dist/lib/commander.js:78:33)
    at RedisClient._RedisClient_sendCommand (/home/stephan/Documents/DEV/backend/node_modules/redis/dist/lib/client.js:360:66)
    at RedisClient.commandsExecutor (/home/stephan/Documents/DEV/backend/node_modules/redis/dist/lib/client.js:111:136)
    at RedisClient.BaseClass.<computed> [as set] (/home/stephan/Documents/DEV/backend/node_modules/redis/dist/lib/commander.js:12:29)
    at RedisStore.set (/home/stephan/Documents/DEV/backend/node_modules/connect-redis/lib/connect-redis.js:65:21)
    at Session.save (/home/stephan/Documents/DEV/backend/node_modules/express-session/session/session.js:72:25)
    at Session.save (/home/stephan/Documents/DEV/backend/node_modules/express-session/index.js:406:15)
    at ServerResponse.end (/home/stephan/Documents/DEV/backend/node_modules/express-session/index.js:335:21) {
  code: 'ERR_INVALID_ARG_TYPE'

It works without any problems if I don't set any value to the session. (as fast as I remove the comment for request.session.views = 1;, I get the above error at response.end().

@johncapfull
Copy link

Workaround for the type issue is adding .toString() to ttl and _getTTL(sess) in following places, as legacy redis currently does not expect number in the command arguments:

args.push('EX', ttl)

this.client.expire(key, this._getTTL(sess), (err, ret) => {

But it seems that issue with numeric arguments should be fixed in node-redis anyway.

@brunoshine
Copy link

Hi all,

any workaround for this?

thanks.

@github-actions
Copy link

This issue 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 Nov 13, 2021
@github-actions
Copy link

This issue was closed because it has been stalled for 5 days with no activity.

@wavded
Copy link
Collaborator

wavded commented Nov 29, 2021

Work for this has begun -> #337

@wavded wavded reopened this Nov 29, 2021
@github-actions github-actions bot closed this as completed Dec 5, 2021
@wavded
Copy link
Collaborator

wavded commented Feb 22, 2023

I am removing the legacyMode requirement in the next major version of this package. If you want to try it you can npm install connect-redis@next in your projects:

Migration guide in this PR: #377

@wavded wavded reopened this Feb 22, 2023
@github-actions
Copy link

This issue was closed because it has been stalled for 5 days with no activity.

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

No branches or pull requests

4 participants