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: allow adding providers when the module is loaded asynchronously #1236

Conversation

jevillard
Copy link
Contributor

@jevillard jevillard commented Apr 4, 2022

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:

It may be a bug fix or a feature, I'm not sure how you consider this PR

What is the current behavior?

Today, when calling the forRootAsync function, it is not possible to define your own providers if necessary

Issue Number: N/A

What is the new behavior?

Here is an example in one of my projects where I want to encapsulate the declaration of the TypeOrmModule module in my own DatabaseModule module so as not to repeat the declaration of my logger or other common options several times. This module is at the root of my Git repository and is used in different NestJS applications that I have in this same Git repository

  • database.module.ts
export type DatabaseModuleOptions = TypeOrmModuleOptions
export type DatabaseModuleAsyncOptions = Pick<ModuleMetadata, 'imports'> & {
  useFactory: (
    ...args: any[]
  ) => DatabaseModuleOptions | Promise<DatabaseModuleOptions>;
  inject?: any[];
};

@Module({})
export class DatabaseModule {
  static forRootAsync(options: DatabaseModuleAsyncOptions): DynamicModule {
    return {
      module: DatabaseModule,
      imports: [
        TypeOrmModule.forRootAsync({
          imports: options.imports,
          providers: [
            {
              provide: 'DATABASE_MODULE_OPTIONS',
              useFactory: options.useFactory,
              inject: options.inject,
            },
          ],
          inject: ['DATABASE_MODULE_OPTIONS', PinoLogger],
          useFactory: (options: DatabaseModuleOptions, logger: PinoLogger) => ({
            ...options,
            loggerLevel: 'debug',
            logger: new TypeOrmLogger(logger),
            entities: [ // ...],
          }),
        } as TypeOrmModuleAsyncOptions),
      ],
      exports: [TypeOrmModule],
    };
  }
  • foo.app.module.ts
@Module({
  imports: [
    DatabaseModule.forRootAsync({
      imports: [ConfigModule],
      useFactory: (configService: ConfigService) => ({
        ...configService.get('app.database.foo'),
      }),
      inject: [ConfigService],
    }),
  ]
})

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

What do you think of this modification to allow to simply add providers?

@jevillard
Copy link
Contributor Author

Anybody to review this little PR 😁 ?
Thanks 😄

@jmcdo29
Copy link
Member

jmcdo29 commented Apr 6, 2022

If I'm understanding this right, this would be a new approach to this problem, correct?

@jevillard
Copy link
Contributor Author

jevillard commented Apr 7, 2022

Yes exactly.

We could also use the principle of the HttpModule of NestJS which in its asynchronous options offers an optional parameter called extraProviders?: Provider[].
What do you think about?

@kamilmysliwiec
Copy link
Member

Yeah if we can rename it to extraProviders that would be great (to remain consistent with other packages)

@jevillard jevillard force-pushed the fix-allow-adding-providers-loading-module-async branch from b366873 to c44b6bc Compare April 7, 2022 11:57
@jevillard
Copy link
Contributor Author

Ok it's done, we can now pass additional providers with the extraProviders attribute

@jevillard
Copy link
Contributor Author

I think you can merge this PR and release a new minor version of the package. Or do you want to add something else?

@jevillard
Copy link
Contributor Author

Up please 😃
With the 3 approvals, I think we can merge this MR.

@kamilmysliwiec kamilmysliwiec merged commit 7a43ef3 into nestjs:master May 20, 2022
@jevillard jevillard deleted the fix-allow-adding-providers-loading-module-async branch May 20, 2022 12:54
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.

6 participants