added loadAsync feature

[lovesharma95]

PR Checklist

Please check if your PR fulfills the following requirements:

• [x] The commit message follows our guidelines: https://github.com/nestjs/nest/blob/master/CONTRIBUTING.md
• [x] Tests for the changes have been added (for bug fixes / features)
• [ ] Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

• [ ] Bugfix
• [x] 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?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

• [x] Yes
• [ ] No

Other information

[lovesharma95]

added the loadAsync option for loading custom configurations asynchronously. Use Case: If the user wants to load env variables from a third party then they can do that now.

[EcksDy]

Hey guys(@lovesharma95 @micalevisk), I had some issues with config module load functionality, which brought me here. I'm trying to understand more about it, so excuse me if I'm totally off.

Are we sure this breaking change makes sense? Modules usually have a sync forRoot and its async version forRootAsync, making a currently sync forRoot might be confusing.

Also the ConfigFactory can already be asynchronous, does it make sense that getting the ConfigFactory is also async? Seems weird, since the need is to load the env variables asynchronously, not the factory itself.

Another small one, is this map necessary? const configFactory = options.load ? await Promise.all(options.load.map((configFactory) => configFactory)) : [];

Thanks!

[micalevisk]

fair enough @EcksDy

I agree that the changes here are not aligned with the 'sync' vs 'async' dynamic modules.

Also, double-reading the changes, I found a bit weird that the factory itself can be a promise. I don't know why we need to support this, exactly. Do you mind on elaborating the use case @lovesharma95 ?

[EcksDy]

@micalevisk Sorry, I wasn't clear. I meant that currently forRoot is sync, the changes in this PR will make it async. This will be confusing, when considering the convention of forRoot and forRootAsync functions :)

[micalevisk]

I agree that the changes here are aligned

I meant are not

[lovesharma95]

@EcksDy @micalevisk The Idea is to wait for the promise to get resolved before any dependent code executes. My use case: I'm facing an issue where my application attempts to utilize environment variables retrieved from Secret Manager before they're fully loaded. This leads to unexpected behavior because other modules within the same file might try to access these variables prematurely

@EcksDy I also agree that adding async in forRoot confuses users because of the convention but adding forRootAsync feature I guess is a big feature request.

That's why I added promise in load only but maybe you can suggest me another solution or just wait for forRootAsync

[micalevisk]

This leads to unexpected behavior because other modules within the same file might try to access these variables prematurely

I think that this is something that could be fixed without more features. I'd have to see a minimum reproduction of your use case tho

[lovesharma95]

steps to reproduce Steps:

1. Config Class:

• Create a Config class with a static load function that mocks data retrieval from Secret Manager and returns a resolved promise containing environment variables.

export class Config { static load() { // Mock data for Secret Manager return Promise.resolve({ dbType: 'mysql', dbName: 'dummy_db', dbUser: 'dummy_user', dbPassword: 'dummy_password', dbHost: 'localhost', dbPort: 3306, dbSync: true }); } }

2. ConfigModule.forRoot:

• In your ConfigModule.forRoot configuration within AppModule, utilize Config.load as a provider function wrapped in an array.

@Module({ imports: [ ConfigModule.forRoot({ load: [() => Config.load()], }), ], }) export class AppModule {}

3. TypeOrmModule Configuration (Error Prone):

• Within the same AppModule, import TypeOrmModule and configure it asynchronously using forRootAsync.
• Inject ConfigService and attempt to access environment variables retrieved by Config.load.

TypeOrmModule.forRootAsync({ imports: [ConfigModule], inject: [ConfigService], useFactory: async (configService: ConfigService) => ({ type: configService.get<string>('dbType'), // ... other configuration using environment variables }), })

Expected Behavior:

• Config.load resolves before TypeOrmModule configuration. Actual Behavior:
• TypeOrmModule attempts to access environment variables before Config.load resolves, resulting in an error. Attempted Solution (Unideal):
• Modify Config to hold loaded data in a static property (config) and update load to assign the resolved promise.
• In main.ts, call Config.load asynchronously before bootstrapping the application.

export class Config { public static config: ConfigProps; constructor() {} public static get() { return Config.config; } static load() { // mock data for secret manager Config.config = Promise.resolve({ dbType: 'mysql', dbName: 'dummy_db', dbUser: 'dummy_user', dbPassword: 'dummy_password', dbHost: 'localhost', dbPort: 3306, dbSync: true }) } }

async function bootstrap() { await Config.load(); const app = await NestFactory.create(AppModule); }

load: [Config.get]

Discussion:

• The current approach is a workaround, not a recommended solution.
• Ideally, config module should offer a mechanism (like forRootAsync) to explicitly wait for promises to resolve before continuing configuration.

[micalevisk]

to me that seems to be a bug

If a provider depends on ConfigService, then that loading step from ConfigService must be resolved before any call to ConfigService#get

Not sure if this is on @nestjs/typeorm side but I've succeeded on using an async loading like you wanted to:

full code

// app.module.ts
import { setTimeout as sleep } from 'timers/promises'
import { Module } from '@nestjs/common'
import { ConfigModule, ConfigService } from '@nestjs/config'

class Config {
  static async load() {
    await sleep(1000 * 3) // 3s

    return {
      foo: 123
    }
  }
}

@Module({
  imports: [
    ConfigModule.forRoot({
      load: [() => Config.load()] // or just [Config.load]
    })
  ],
  providers: [
    {
      provide: 'bar',
      inject: [ConfigService],
      useFactory(configService: ConfigService) {
        console.log("foo value from bar:", configService.get('foo'))
      }
    }
  ],
})
export class AppModule {}

output:

[lovesharma95]

not just @nestjs/typeorm but it is also behaving the same in other imported modules like CacheModule from @nestjs/cache-manager and LoggerModule from nestjs-pino . This is happening when trying to use it with other packages in the imports array. In providers it is working fine for me as well

[micalevisk]

I'd suggest you to open a bug report ticket instead then

I still believe that there is some sort of misusage tho

[EcksDy]

I've had a similar issue, there is a bug with how you configured TypeOrm:

TypeOrmModule.forRootAsync({ imports: [ConfigModule], // <--- THIS will override the global ConfigModule with a default one, remove this line to get the behaviour you expect 
inject: [ConfigService], useFactory: async (configService: ConfigService) => ({ type: configService.get<string>('dbType'), // ... other configuration using environment variables }), })

There is another bug with that approach though. I have a min repro for it, just need to open the issue. The injected ConfigService will wait for the async loader, but will eventually resolve with values from .env file or the envars of the shell. ignoreEnvFile flag is ignored. In case both of them(.env file and envars) are not there, then it will resolve from the async loader correctly.

[lovesharma95]

Thanks @EcksDy It worked not just for typeorm but for other modules as well imports: [ConfigModule] override the global config module.

[kamilmysliwiec]

lgtm

[EcksDy]

@kamilmysliwiec Have you read the conversation above? Is there actually a good reason for adding another way to do this asynchronously?

[kamilmysliwiec]

Yes, I did. Example: async validators in custom namespaced configs

[EcksDy]

@kamilmysliwiec wouldn't a better approach be to add a validation step that uses validation schemas to where the async loaders are resolved? That way no API is broken, and the use-case is handled, or I'm missing something?

P.S. When saying custom namespaces you mean not global configs?

Still feels like the forRoot should stay sync.

[kamilmysliwiec]

Still feels like the forRoot should stay sync.

Whether it stays sync or becomes async doesn't change anything (there's no difference in behavior anyway)