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
added loadAsync feature #1682
base: master
Are you sure you want to change the base?
added loadAsync feature #1682
Conversation
added support for load custom configuration files asynchronously
added the loadAsync option for loading custom configurations asynchronously. |
Hey guys(@lovesharma95 @micalevisk), Are we sure this breaking change makes sense? Also the Another small one, is this map necessary? Thanks! |
@@ -88,7 +88,8 @@ export class ConfigModule { | |||
} | |||
|
|||
const isConfigToLoad = options.load && options.load.length; | |||
const providers = (options.load || []) | |||
const configFactory = options.load ? await Promise.all(options.load.map((configFactory) => configFactory)) : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const configFactory = options.load ? await Promise.all(options.load.map((configFactory) => configFactory)) : []; | |
const configFactory = await Promise.all(options.load || []); |
remove the redundancy
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 ? |
@micalevisk Sorry, I wasn't clear. |
I meant are not |
@EcksDy @micalevisk The Idea is to wait for the promise to get resolved before any dependent code executes. @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 |
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 |
steps to reproduce 1. Config Class:
2. ConfigModule.forRoot:
3. TypeOrmModule Configuration (Error Prone):
Expected Behavior:
Discussion:
|
to me that seems to be a bug If a provider depends on Not sure if this is on 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: |
not just |
I'd suggest you to open a bug report ticket instead then I still believe that there is some sort of misusage tho |
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. |
Thanks @EcksDy It worked not just for typeorm but for other modules as well |
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information