Skip to content

RedisClientType hard to use with createClient #1865

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
tobiasdiez opened this issue Jan 21, 2022 · 25 comments
Closed

RedisClientType hard to use with createClient #1865

tobiasdiez opened this issue Jan 21, 2022 · 25 comments

Comments

@tobiasdiez
Copy link

tobiasdiez commented Jan 21, 2022

The RedisClientType has two generic parameters that make it hard to use to signify the return type of createClient. In particular, the first argument modules in the declaration of createClient is a local variable that is not exported and thus cannot be easily reused in one's own code.

Maybe it's already enough to provide sufficiently general defaults for the generic arguments. If I'm not mistaken these parameters control more the configuration of the client and don't really influence the typing of the "normal" methods; so they are not that important for an user of a factory method.

Below is a similar observation from #1673.


The RedisClientType exported since 4.0.1 still does not work for me. Could not figure out how to make the exported RedisClientType, RedisClientOptions and the type returned by createClient compatible. Here is an example that illustrates the issue.

The following does not compile unless someone knows how to type RedisClientOptions correctly (refer to the code sandbox link above).

import { createClient, RedisClientOptions, RedisClientType } from "redis";

const factory = (options: RedisClientOptions<any, any>): RedisClientType => {
  return createClient(options);
};

const client: RedisClientType = factory({...});

Instead, I am doing this for now:

import { createClient} from "redis";
type RedisClientType = ReturnType<typeof createClient>;
type RedisClientOptions = Parameters<typeof createClient>[0];

const factory = (options: RedisClientOptions): RedisClientType => {
  return createClient(options);
};

const client: RedisClientType = factory({...});

Originally posted by @tbinna in #1673 (comment)

@leibale
Copy link
Contributor

leibale commented Jan 24, 2022

RedisModules extends the client with the installed modules commands, RedisScripts extends it with the defined lua scripts (see here).

In case you want to ignore modules and scripts, Record<string, never> should do the trick:

RedisClientType<Record<string, never>, Record<string, never>>

I guess my comment should be added to the docs somewhere, I'll leave this issue open for now

@tobiasdiez
Copy link
Author

Thanks! Would it then be an idea to set these generic parameters to Record<string, never> by default, i.e. RedisClientType without any generics specified is equivalent to what you wrote above?

@restfulhead
Copy link

@leibale Could you provide an example how to use a typed client with scripts?

What works for me without defining scripts:

export type RedisClientType = ReturnType<typeof createClient>

myCreateClient(port, host, pw): RedisClientType {
 const client: RedisClientType = createClient({
      socket: { port, host, tls: true },
      password: pw,
    })
  return client
}

However, once I add a script, I get a types are incompatible error.

myCreateClient(port, host, pw): RedisClientType {
 const client: RedisClientType = createClient({
      socket: { port, host, tls: true },
      password: pw,
      scripts: {
        add: defineScript({
          NUMBER_OF_KEYS: 1,
          SCRIPT: 'local val = redis.pcall("GET", KEYS[1]);' + 'return val + ARGV[1];',
          transformArguments(key: string, toAdd: number): string[] {
            return [key, toAdd.toString()]
          },
          transformReply(reply: number): number {
            return reply
          },
        }),
      },
    })
  return client
}

@leibale
Copy link
Contributor

leibale commented Jan 25, 2022

@restfulhead

import { createClient, defineScript, RedisModules, RedisScripts, RedisClientType } from '@node-redis/client';

interface ClientOptions<
    M extends RedisModules,
    S extends RedisScripts
> {
    port: number;
    host: string;
    pw: string;
    modules?: M;
    scripts?: S;
}

function clientFactory<
    M extends RedisModules,
    S extends RedisScripts
>(options: ClientOptions<M, S>): RedisClientType<M, S> {
    return createClient({
        socket: {
            port: options.port,
            host: options.host
        },
        password: options.pw,
        modules: options.modules,
        scripts: options.scripts
    });
}

const client = clientFactory({
    port: 6379,
    host: 'host',
    pw: 'pw',
    scripts: {
        add: defineScript({
          NUMBER_OF_KEYS: 1,
          SCRIPT: 'local val = redis.pcall("GET", KEYS[1]);' + 'return val + ARGV[1];',
          transformArguments(key: string, toAdd: number): Array<string> {
            return [key, toAdd.toString()]
          },
          transformReply(reply: number): number {
            return reply
          },
        })
    }
});

client.add('key', 1);
//     ^?

playground

leibale added a commit to leibale/node-redis that referenced this issue Jan 31, 2022
@restfulhead
Copy link

restfulhead commented Jan 31, 2022

Hm...thanks for the example, but what I really need is a factory that returns the type of the configured client; not the generic one. My application will use the same client everywhere, so I need a type to pass around the configured client.

For example say I'd like to pass my client to a function:

// how to best define the MyClient type here?
function doAdd(client: MyClient) {
   client.add('key', 1);
}

Now adding on to your example, the following works, but requires a lot of declaration:

export type MyClient = RedisClientType<RedisModules, {
    add: {
        NUMBER_OF_KEYS: number;
        SCRIPT: string;
        transformArguments(this: void, key: string, toAdd: number): Array<string>;
        transformReply(this: void, reply: number): number;
    } & 
    { 
        SHA1: string;
    }
}>

@leibale Would it be feasible to export the RedisScript and SHA1 types?

This would make the declaration (and therefore using) this library much easier:

export interface ConfiguredRedisScripts extends RedisScripts {
  add: RedisScript
}
export type MyClient = RedisClientType<RedisModules, ConfiguredRedisScripts>

function doAdd(client: MyClient) {
   client.add('key', 1);
}

@leibale
Copy link
Contributor

leibale commented Feb 1, 2022

@restfulhead

import { createClient } from '@node-redis/client';

export const client = createClient({
  modules: {
    // ...
  },
  scripts: {
    // ...
  }
});

export type CustomRedisClient = typeof client;

const duplicate = client.duplicate();
import { createClient } from '@node-redis/client';

const modules = {
  // ...
};

const scripts = {
  // ...
};

export type CustomRedisClient = RedisClientType<typeof modules, typeof scripts>;

export function clientFactory(): CustomRedisClient {
  return createClient({
    modules,
    scripts
  });
}

@tobiasdiez
Copy link
Author

@leibale thanks for adding the defaults to the RedisClientType params. However, now the following code

import redis, { RedisClientType } from 'redis'
function createRedisClient(): Promise<RedisClientType> {
    const client = redis.createClient(...redisConfig...)
    await client.connect()
    return client
}

fails with an error of the type

error TS2322: Type 'RedisClientType<{ graph: { CONFIG_GET: typeof import("/home/runner/work/JabRefOnline/JabRefOnline/node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("/home/runner/work/JabRefOnline/JabRefOnline/node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: typeo...' is not assignable to type 'RedisClientType<Record<string, never>, Record<string, never>>'.

@leibale
Copy link
Contributor

leibale commented Feb 14, 2022

@tobiasdiez #1984 should fix it

@tobiasdiez
Copy link
Author

Thanks! I haven't had a chance to try it, but codewise it looks good to me.

@tobiasdiez
Copy link
Author

I hate to bother you again, but the code from above (#1865 (comment)) is still not working and now fails with

Type 'import("file:///node_modules/@node-redis/client/dist/lib/client/index").RedisClientType<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: typeo...' is not assignable to type 'import("file:///node_modules/redis/dist/index").RedisClientType<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: typeof import("file:///node_m...'.
  Type 'RedisClientType<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: typeof import("file:///node_modules/@node-redis/graph/dist/commands/SLOWLOG")...' is not assignable to type 'RedisClient<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: typeof import("file:///node_modules/@node-redis/graph/dist/commands/SLOWLOG"); };...'.
    Types of property 'options' are incompatible.
      Type 'import("file:///node_modules/@node-redis/client/dist/lib/client/index").RedisClientOptions<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: ty...' is not assignable to type 'import("file:///node_modules/@node-redis/client/dist/lib/client/index").RedisClientOptions<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: ty...'. Two different types with this name exist, but they are unrelated.
        Type 'import("file:///node_modules/@node-redis/client/dist/lib/client/index").RedisClientOptions<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: ty...' is not assignable to type 'import("file:///node_modules/@node-redis/client/dist/lib/client/index").RedisClientOptions<{ graph: { CONFIG_GET: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); configGet: typeof import("file:///node_modules/@node-redis/graph/dist/commands/CONFIG_GET"); ... 15 more ...; slowLog: ty...'. Two different types with this name exist, but they are unrelated.
          Type 'RedisScripts' is not assignable to type 'Record<string, never>'.
            'string' index signatures are incompatible.
              Type 'RedisScript' is not assignable to type 'never'.(2322)

See https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBFApgE2AZwDRwN5wEoroDCANsIgHYwAqAnmInAL5wBmUEIcA5EqmtwBQAQzS0KAYzYBXSTGAQKcCUmExEBfqXJUAFAEoAXHAAKHEOkQAeTcTKUa9RAD4cguB+WK08CfapwALwIhGgAdCqIaojaDgbunjBOyv6ODEFwSQwQrCk6MAkeWYwA4ogwAMoqwGBp1kRwiAAe6hTIaPihsVR0DFbCFLRYA7TOrsENza3tnVqpvdYjWMAUrIhQcNSuAPybcMayANYUEADuFADchZnJaNW1Cxlllfd1Vn75C87XAPQ-nh4AHrba7CU7CYC+VIRRQURASGDxAFIGDSKBKD4OQRMIA

The issue here is that the script type returned by createClient is redis.RedisScript but the default script type for RedisClientType is Record<string, never>. Would it be possible to change the default?

@pranshu-07
Copy link

pranshu-07 commented Jun 14, 2022

@leibale any update on this, it's not working since v4.1.0, #1865 (comment)

@tobiasdiez
Copy link
Author

@leibale can this issue please be reopened, it is still not working as reported above #1865 (comment). Thanks!

@leibale
Copy link
Contributor

leibale commented Aug 21, 2022

@tobiasdiez the default is that there are no scripts, why is that a problem?

@tobiasdiez
Copy link
Author

tobiasdiez commented Aug 21, 2022

The issue is that if you do use scripts, then you need to specify the type of the scripts everywhere. Instead of "no scripts", something like "generic scripts" would be helpful. The goal is to make the following code work

import redis, { RedisClientType } from 'redis'
function createRedisClient(): Promise<RedisClientType> {
    const client = redis.createClient(...redisConfig...)
    await client.connect()
    return client
}

function something(client: RedisClientType) {}
something(await createRedisClient())

Currently, one need to use RedisClientType<any, any, any> everywhere.

@tobiasdiez
Copy link
Author

Thanks but this is not really what I need. My requirements (and I believe many others are in a similar position) are as follows:

  • Have a factory method in one module, that creates a redis client. (This factory method is then registered in the dependency framework)
  • In other modules, I would like to use the redis client, i.e. as a constructor or method argument.

In both instances, I'm not really able to easily type the redis client. I could pass down the generics as you suggest with your generic function, but that is unhandy since now every other method that uses the redis client needs these generics - but in my application I only have one redis client so the generics are actually fixed.

The following code works

import redis, { RedisClientType } from 'redis'
type RedisClient = RedisClientType<redis.RedisModules, redis.RedisFunctions, redis.RedisScripts>
async function createRedisClient(): Promise<RedisClient> {
    const client = redis.createClient()
    await client.connect()
    return client
}

My suggestion is that the generics of RedisClientType default to <redis.RedisModules, redis.RedisFunctions, redis.RedisScripts> so that one can use RedisClientType directly over RedisClient.
By the way, the redis client returned by createClient has more modules than redis.RedisModules. I think redis.RedisModules should be the same as what createClient returns without any args.
Another suggestion: rename RedisClientType to RedisClient, the type suffix isn't necessary and its clear from the context that its a type.

@leibale
Copy link
Contributor

leibale commented Aug 22, 2022

  1. I can't rename RedisClientType to RedisClient because:
    1.1. RedisClient is the name of the class
    1.2. It'll be a breaking change
  2. RedisModules is just a generic type from @redis/client, don't confuse it with the default modules in redis, which I should probably export as well - wanna open an issue about it?
  3. Why do you think that every method will need these generics? see this example

@tobiasdiez
Copy link
Author

  1. Makes sense, okay!
  2. Done: Types: Expose default modules #2241
  3. What I meant is that function accepting a RedisClientType would then need generic arguments as well, e.g.
 function store<M, F, S>(client: RedisClientType<M, F, S>, value: string) {
    // use client here...
 } 

This gets ugly quickly.

@leibale
Copy link
Contributor

leibale commented Aug 22, 2022

regarding 3 - you can use the typescript typeof operator

const client = createClient({
  // ...
});

type MyRedisClient = typeof client;

@tobiasdiez
Copy link
Author

Yes, but only if you don't use a factory method to create the client, since then client is only visible in that factory method and thus its type cannot be exported to other modules (or used to annotate the return type of the factory).

@thatcort
Copy link

Just came here to say that all of this seems far too hard and worthy of a breaking change to the type definitions. After struggling with this for far too long I just put const client = redis.createClient(redisClientOptions) as RedisClientType<any, any, any> in my factory function.

@bowmana
Copy link

bowmana commented Jun 8, 2023

This worked for me:

import  Redis from "ioredis";
const redisURL = process.env.REDIS_URL || "redis://localhost:6379";
const redisClient = new Redis(redisURL);
redisClient.on("error", (err) => {
    console.error("Error connecting to Redis:", err);
});


//example code below with get

app.get("/:user_id/getrecipes", async (req: Request, res: Response) => {
    const user_id : number = parseInt(req.params.user_id);
    const cacheKey = `user:${user_id}:recipes`;
    redisClient.get(cacheKey, async (err, cachedData) => {
        if (err) {
            console.error("Error retrieving data from Redis cache:", err);
            res.status(500).send("Error getting recipes from cache");
            return;
        }

        if (cachedData) {
            const recipes = JSON.parse(cachedData);
            res.status(200).send(recipes);
        } else {
            try{...

@clemenspeters
Copy link

Thanks @thatcort , I my case just const client = redis.createClient(redisClientOptions) as RedisClientType was sufficient already.

@Kaskyi
Copy link

Kaskyi commented Aug 4, 2024

I've encountered the same typing issue.

Fix:

  1. Install Redis with "redis": "^5.0.0-next.4".
  2. Use:
import { createClient, RedisClientType } from 'redis';

const client: RedisClientType = createClient({ url: '' });

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 a pull request may close this issue.

9 participants