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

feat(common): pass options to nested async modules #9935

Conversation

random42
Copy link

@random42 random42 commented Jul 16, 2022

Pass parent module providers to 'provideInjectionTokensFrom' in ConfigurableModuleAsyncOptions in order to pass options down in nested async modules.
Only necessary providers are taken by recursively looking on the 'inject' array.

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Right now it is not easy to have a hierarchy of async modules because options' providers are not passed down the hierarchy. This is an example of the problem. The solution I gave (pushing the options' providers after creating the module) breaks as soon you have more than one level of nesting.

What is the new behavior?

In an ideal application all modules should be imported asynchronously and each options' provider should be passed down the hierarchy of modules in order to resolve every inject dependency.
So I added an optional provideInjectionTokensFrom to the interface ConfigurableModuleAsyncOptions, which takes the parent module's providers, and only injects in the child module those providers that are recursively required by the child module inject array (usually the parent options' provider).
Right now you can implement my new solution with the extras feature, but since this use case is basically universal I think it should be part of the core feature.
Now you can define a hierarchy of modules like this:

import { ConfigurableModuleBuilder, Module } from '@nestjs/common';

interface AOptions {
  a: string;
}

export const {
  ConfigurableModuleClass: A_Module,
  MODULE_OPTIONS_TOKEN: A_TOKEN,
  ASYNC_OPTIONS_TYPE: A_ASYNC_OPTIONS_TYPE,
} = new ConfigurableModuleBuilder<AOptions>({
  moduleName: 'A',
}).build();

export class A extends A_Module {}

interface BOptions {
  b: string;
}

export const {
  ConfigurableModuleClass: B_Module,
  MODULE_OPTIONS_TOKEN: B_TOKEN,
  ASYNC_OPTIONS_TYPE: B_ASYNC_OPTIONS_TYPE,
} = new ConfigurableModuleBuilder<BOptions>({
  moduleName: 'B',
}).build();

export class B extends B_Module {
  static registerAsync(opt: typeof B_ASYNC_OPTIONS_TYPE) {
    const m = super.registerAsync(opt);
    m.imports ??= [];
    m.imports.push(
      A.registerAsync({
        useFactory: (opt: BOptions) => {
          return { a: opt.b };
        },
        inject: [B_TOKEN],
        // the B_TOKEN provider and his dependencies
        // (inject array, in this example the C_TOKEN provider)
        // will be present in the A module providers
        provideInjectionTokensFrom: m.providers,
      }),
    );
    return m;
  }
}

interface COptions {
  c: string;
}

export const {
  ConfigurableModuleClass: C_Module,
  MODULE_OPTIONS_TOKEN: C_TOKEN,
  ASYNC_OPTIONS_TYPE: C_ASYNC_OPTIONS_TYPE,
} = new ConfigurableModuleBuilder<COptions>({
  moduleName: 'C',
}).build();

export class C extends C_Module {
  static registerAsync(opt: typeof C_ASYNC_OPTIONS_TYPE) {
    const m = super.registerAsync(opt);
    m.imports ??= [];
    m.imports.push(
      B.registerAsync({
        useFactory: (opt: COptions) => {
          return { b: opt.c };
        },
        inject: [C_TOKEN],
        // the C_TOKEN provider and his dependencies (inject array)
        // will be present in the B module providers
        provideInjectionTokensFrom: m.providers,
      }),
    );
    return m;
  }
}

The only problem is when options' tokens have the same name string down the path. This can be avoided by using symbols as tokens.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Pass parent module providers to 'provideInjectionTokensFrom' in
ConfigurableModuleAsyncOptions in order
to pass options down in nested async modules.
Only necessary providers are taken
by recursively looking on the 'inject' array.
@coveralls
Copy link

Pull Request Test Coverage Report for Build b658d291-262f-4540-945f-84bbac15a090

  • 17 of 17 (100.0%) changed or added relevant lines in 2 files are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage increased (+7.0e**-05%**) to 94.089%

Files with Coverage Reduction New Missed Lines %
packages/common/module-utils/configurable-module.builder.ts 1 89.52%
Totals Coverage Status
Change from base Build 989d7a11-e284-4a37-8a57-6d51089adf11: 7.0e-05%
Covered Lines: 6065
Relevant Lines: 6446

💛 - Coveralls

@kamilmysliwiec
Copy link
Member

In an ideal application all modules should be imported asynchronously and each options' provider should be passed down the hierarchy of modules in order to resolve every inject dependency.

This isn't correct.

Right now it is not easy to have a hierarchy of async modules because options' providers are not passed down the hierarchy.

Can you please share an example (real-world code example) that illustrates the problem? What are you trying to accomplish that isn't currently feasible?

@random42
Copy link
Author

random42 commented Jul 18, 2022

Sure.

Let's say I have this type of application (this is not runnable, just an example):

@Module({
  imports: [
    ConfigModule,
    AuthenticationModule.registerAsync({
      imports: [ConfigModule],
      useFactory(config: ConfigService) {
        return config.authentication;
      },
      inject: [ConfigService],
    }),
  ],
})
class AppModule {}

class AuthenticationModule {
  static registerAsync(opt) {
    //...
    return {
      providers: [...this.getAsyncProviders(opt)],
      imports: [
        // this is not working because AuthenticationOptions' provider is not present in the JwtModule
        JwtModule.registerAsync({
          useFactory(opt: AuthenticationOptions) {
            return opt.jwtOptions;
          },
          inject: [AuthenticationOptions],
        }),
      ],
    };
  }
}

The reason why I say that all modules should be registered asynchronously is because each module should have its own options, and those options will determine the options of the child modules in a decoupled way: I don't want my AuthenticationModule to know about my ConfigService, that's why I use an async import; same thing for the JwtModule. That is how synchronous modules behave. But since we use a ConfigModule, we are then forced to use async modules down the hierarchy in order to inject the options.

This example breaks because the JwtModule does not have the AuthenticationOptions provider in its providers. I cannot add my AuthenticationModule to the imports because that would be a circular dependency. The only solution is to add the AuthenticationOptions provider the JwtModule's providers. But this will also break because the AuthenticationOptions provider needs the ConfigService, which is also not part of the JwtModule. I hope you see what I mean. That's why I want something that resolves all my inject dependencies recursively by passing down all options' providers.

@kamilmysliwiec
Copy link
Member

If your AuthModule imports the JwtModule, then it creates a unidirectional relationship. Now if you make JwtModule dependent on the AuthenticationOptions you construct a bidirectional relationship (circular dependency) which is considered a bad practice in NestJS. If you want to leverage options passed into AuthModule within JwtModule, you should explicitly pass these options down (and so there will be a corresponding provider registered within JwtModule scope).

So, just to make sure that I'm on the same page, the solution proposed in this PR tends to solve this issue by exposing an additional property that let you duplicate the parent module's providers within the scope of the child module so they can be used to construct its own configuration. Is that correct?

@random42
Copy link
Author

That's correct. I'll point out again that only the providers required to construct the child module's options will be provided to the child module.

@CatsMiaow
Copy link
Contributor

CatsMiaow commented Jul 28, 2022

I had the same problem and I tried as below.

await Test.createTestingModule({
  imports: [MongoModule.forRootAsync({
    imports: [ConfigModule.forRoot({ ... })],
    useFactory: (config: ConfigService) => ({ ...config.get('mongo') }),
    inject: [ConfigService],
  })],
}).compile();

/* mongo.module-definition.ts */
export const {
  ConfigurableModuleClass,
  MODULE_OPTIONS_TOKEN,
} = new ConfigurableModuleBuilder<MongoOptions>().setClassMethodName('forRoot').build();

/* mongo-config.service.ts */
@Injectable()
export class MongooseConfigService implements MongooseOptionsFactory {
  constructor(@Inject(MODULE_OPTIONS_TOKEN) private mongo: MongoOptions) {
    // Logs are output normally when defining services in providers.
    console.log('MongooseConfigService', mongo);
  }

  public createMongooseOptions(): MongooseModuleOptions {
    return { ... };
  }
}

/* mongo.module.ts */
import { MongooseModule } from '@nestjs/mongoose';
...

@Module({
  imports: [
    // first try
    // Nest can't resolve dependencies of the MongooseConfigService (?). Please make sure that the argument CONFIGURABLE_MODULE_OPTIONS[19a70477-4fdc-45fc-9609-1cafb87b7f85] at index [0] is available in the MongooseCoreModule context.
    MongooseModule.forRootAsync({
      useClass: MongooseConfigService,
      inject: [MODULE_OPTIONS_TOKEN],
    }),
    // second try
    // Nest can't resolve dependencies of the MongooseModuleOptions (?). Please make sure that the argument MongooseConfigService at index [0] is available in the MongooseCoreModule context.
    MongooseModule.forRootAsync({
      useFactory: (config: MongooseConfigService) => config.createMongooseOptions(),
      inject: [MongooseConfigService],
      // Nest can't resolve dependencies of the MongooseModuleOptions (?, MongooseConfigService). Please make sure that the argument CONFIGURABLE_MODULE_OPTIONS[31d160a5-c894-4ebf-87f6-bdfc90a8afd2] at index [0] is available in the MongooseCoreModule context.
      // inject: [MODULE_OPTIONS_TOKEN, MongooseConfigService],
    }),
  ],
  providers: [MongooseConfigService],
})
export class MongoModule extends ConfigurableModuleClass {}

@kamilmysliwiec
Copy link
Member

LGTM

@kamilmysliwiec kamilmysliwiec merged commit 23fa0f9 into nestjs:master Jul 28, 2022
@random42 random42 deleted the feature/configurable-module-inject-providers branch July 28, 2022 09:15
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.

None yet

4 participants