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

Update Typescript definitions to be more specific #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MikeRossXYZ
Copy link

@MikeRossXYZ MikeRossXYZ commented Nov 16, 2022

Should fix issue #53 by updating Typescript definitions in a non-breaking way.

Within NestJS, when registering the CacheModule, you can configure it as such:

    CacheModule.registerAsync({
      isGlobal: true,
      useFactory: async (configService: ConfigService) => {
        return {
          store: await redisStore({
            socket: {
              host: configService.get('REDIS_HOST'),
              port: configService.get('REDIS_PORT'),
            },
          }),
        };
      },
    }),

In the future, issue #52 should be addressed by updating the function signatures in the code to properly match Store. This change is a stop-gap measure to unblock users of NestJS framework (and likely others).

@nikensss
Copy link

nikensss commented Nov 24, 2022

EDIT:
Sorry, I fixed that by modifying the return value of the useFactory. I spread store instead:

return { ...store, ttl:60 * 60 * 24 };

Hi! I have been working with this recently. I need to disconnect from Redis when the application shuts down, or else the tests won't finish automatically, and found that the typings are slightly different from what I expected. In this screenshot you can see that, in order to be able to do getClient you need to do cache.store.store. This screenshot belongs to the debugger stopped at the console.log line in the constructor of the AppModule class.

Screenshot 2022-11-24 at 11 08 07

This is what the code I wrote looks like (I stripped some parts to make it shorter):

import {
  CacheModule,
  CacheStore,
  CACHE_MANAGER,
  Inject,
  MiddlewareConsumer,
  Module,
  OnModuleDestroy,
} from '@nestjs/common';
import { Cache } from 'cache-manager';
import { redisStore } from 'cache-manager-redis-store';
/* more imports here */

@Module({
  imports: [
    /* other module imports and config here */
    CacheModule.registerAsync({
      imports: [ConfigModule],
      inject: [ConfigService],
      isGlobal: true,
      useFactory: async (config: ConfigService) => {
        const store = await redisStore({ url: config.get('REDISCLOUD_URL') });
        return { store: store as unknown as CacheStore, ttl: 60 * 60 * 24 };
      },
    }),
  ],
})
export class AppModule implements OnModuleDestroy {
  constructor(@Inject(CACHE_MANAGER) private cache: Cache) {
    console.log('constructor app module');
  }

  configure(consumer: MiddlewareConsumer): void {
    consumer.apply(RequestLoggerMiddleware).forRoutes('*');
  }

  onModuleDestroy(): void {
    //eslint-disable-next-line
    const store = this.cache.store as any;
    store.store
      .getClient()
      .disconnect()
      .catch((ex: unknown) => {
        console.log({ ex });
        this.onModuleDestroy();
      });
  }
}

@MikeRossXYZ
Copy link
Author

MikeRossXYZ commented Nov 28, 2022

Are the types in this PR incorrect or is the code snippet example for setting up Nest? Just want to make sure I know what to fix (if anything)!

Side note: The double assertion you have in your Nest config (store as unknown as CacheStore) defeats the purpose of Typescript and effectively forces the compiler to ignore the type error. For applications where security is a consideration, introducing unsafe code like that wouldn't be allowed by many organizations.

@nikensss
Copy link

To be honest, I don't know whose responsibility would be to fix that. I'm not sure it belongs on your PR, but maybe the admin of this repo, when looking at this PR, might pick it up.

About the double assertion, yes, I am aware of that but it was the only way I found to make it work. This has bitten me back just today, when after updating @nestjs/{common,core}@9.2.0, it seems the cache object has only the known properties from the interface, and the rest are stripped. I could no longer do .getClient(). Downgrading to 9.0.2 to fixed it.

Sorry for being so unhelpful. :(

@BerniWittmann
Copy link

@MikeRossXYZ Just stumbled upon this PR and wanted to let you know that i had to adjust the types as following for my typescript not to complain anymore (uppercase Function, adjusting cb type for reset, keys and ttl methods, and cb of reset being optional. return type of keys)

interface RedisStore extends Store {
    name: string;
    getClient: () => RedisClientType;
    isCacheableValue: any;
    set: <T>(key: any, value: T, options?: any, cb?: Function) => Promise<void>;
    get: <T>(key: any, options?: any, cb?: Function) => Promise<T | undefined>;
    del: (...args: any[]) => Promise<any>;
    mset: (...args: any[]) => Promise<any>;
    mget: (...args: any[]) => Promise<any>;
    mdel: (...args: any[]) => Promise<any>;
    reset: (cb?: Function) => Promise<any>;
    keys: (pattern: string, cb?: Function) => Promise<string[]>;
    ttl: (key: any, cb?: Function) => Promise<any>;
}

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.

3 participants