Improve the Client Provider initialization

[xavierchow]

Problem

I'm not sure if this is expected behavior but I figured out the usages of InjectRedis needs to be prior to the RedisModule.forRoot,
otherwise, it throws an unresolved dependency error.

Digging into the code, it seems createRedisClientProviders relies on the namespaces,
while the namespaces only got set within InjectRedis decorator

nestjs-redis/lib/redis/common/redis.decorator.ts

Line 15 in 499d486

namespaces.set(namespace, token);

That means it doesn't work if I want to use a lazy-inject with decorators after the RedisModule's initialization(forRoot).
I submitted a branch here: https://github.com/xavierchow/nestjs-redis/tree/dependency-issue/sample/01-testing-inject to reproduce the issue. (you can try to npm run the 01-testing-inject sample project)

Proposal

Instead of the namespaces set by the decorators, we can use the options.config.namespace to create providers in the createRedisClientProvider function.

@liaoliaots does it sound good to you? I'm ok with sending a PR if needed.

[liaoliaots]

Thanks. This is a good suggestion, I'm testing it, and if ok I will publish a new fix version.

[liaoliaots]

There is not way to use options.config.namespace in createRedisClientProviders because everything is provider.

Dynamic modules can create the module settings (which are usually given as parameter of the @Module-decorator) via a method. This method will get called after the @InjectRedis calls and therefore array namespaces will contain all the namespace used. I think this is only way to create redis instance providers.

According to mechanism of Dynamic modules, in some cases(register RedisModule in other file), you must confirm your import in correctly order OR register RedisModule into root module directly. And try it again.

But, what is lazy-inject, a lib? or a syntax?

Would you open a pull request for this issue?

[xavierchow]

sure, I will try to send a pull request.

But, what is lazy-inject, a lib? or a syntax?

Sorry for the confusion, I meant the @Inject decorator is actually a helper that helps the nest framework to initialize the dependencies, it should not be a prerequisite of a dynamic Module booting.
In my case, it behaves as a lazy (or later) inject decorator applied on the other module after the RedisModule's initialization.

[xavierchow]

Ah, I see the challenge.
It's easy to define providers when RedisModule is initialized with forRoot by updating the createRedisClientProviders as follows,

export const createRedisClientProviders = (options: RedisModuleOptions): FactoryProvider<Redis>[] => {
    let configs: RedisClientOptions[] = [];
    if (options.config) {
        if (Array.isArray(options.config)) {
            configs = options.config;
        } else {
            configs = [options.config];
        }
    }

    const providers: FactoryProvider<Redis>[] = [];
    configs.forEach(config => {
        const namespace = config.namespace ?? DEFAULT_REDIS_NAMESPACE;
        const token = getRedisToken(namespace);
        providers.push({
            provide: token,
            useFactory: (redisManager: RedisManager) => redisManager.getClient(namespace),
            inject: [RedisManager]
        });
    });

    return providers;
};

However, in the case of forRootAsync, what we are able to get when defining providers is just RedisModuleOptionsAsync that only returns RedisModuleOptions when nestjs starts to resolving dependencies, i.e. there are no static options.config.namespace at the moment.
This means those tokens in Line:107 are unknown when defining the providers for InjectRedis decorators.

nestjs-redis/lib/redis/redis.providers.ts

Lines 106 to 109 in 499d486

	providers.push({
	provide: token,
	useFactory: (redisManager: RedisManager) => redisManager.getClient(namespace),
	inject: [RedisManager]

@liaoliaots I think only updating the forRoot doesn't bring a solution as a whole and there are indeed some workarounds, e.g.

1. use RedisService instead of decorators
2. make sure the InjectRedis prior to the RedisModule booting.

I'm gonna close this ticket. (It's on your call to see if it's better to update the REAMDE to recommend using RedisService over InjectRedis decorator to avoid this pitfall.)

[clintonb]

I'm gonna close this ticket. (It's on your call to see if it's better to update the REAMDE to recommend using RedisService over InjectRedis decorator to avoid this pitfall.)

I just wasted over an hour on this issue. @InjectRedis() is unusable from my perspective and should NOT be the first option new users see for injecting the client.

[MarkoXR]

Same issue as @clintonb , solved it using RedisService. This should be more visible. Especially since it worked that way with ioredis and people are migrating from it to this library. Other than that this was practically a drop-in replacement :)