bashleigh / nestjs-config-v2-prototype

An example/prototype of v2 for nestjs-config
7 stars 1 forks source link

Release? #4

Open ghost opened 5 years ago

ghost commented 5 years ago

Do you know when this feature is going to be merged into the nestjs-config?

bashleigh commented 5 years ago

well you can use it now! With nestjs-config@next I'm too scared to make it the latest! :joy: Please let me know what you think! And if you find any bugs please let me know! :)

ghost commented 5 years ago

You tested it already in some projects? It would be really cool to make it the latest. The behaviour before was not really helpful, because injecting the ConfigService in every class where external config is necessary makes no sense - the classes should only depend on their corresponding configuration-object/class. So having multiple providers (one per configuration) should be the most important feature of this library.

bashleigh commented 5 years ago

Yes! It was something I really wanted to address as like you said injecting the ConfigService everywhere was a bit of a pain and really unnecessary. I know of a bug in the library which I've been struggling to find the time to fix unfortunately which is putting me off deploying this. Plus I'm scared if I make v2 the @latest nobody will like it.

The bug seems to be related to using both exported objects and defined classes. I'm not sure why but the configService doesn't seem to be able to find the tokens if you use both. I might just deploy this anyway and see if I can get around to fixing it.

Did you try v2? If so what did you think?

ghost commented 5 years ago

At the moment I am having problems invoking the ConfigModule. You changed the signature?

const configOptions = {
  modifyConfigName:
    (name) => name.replace(".config", ""),
  path:
    Path.resolve(__dirname, "../env", !env ? ".env.local.yml" : ""),
};

@Module({
  imports: [
    ConfigModule.load(
      Path.resolve(__dirname, "**/!(*.d).config.{ts,js}"), configOptions,
    )
  ],
})
export class AppModule {}

This code snippet is not working anymore based on your beta-version. There is no load-method anymore. And all the left methods do not provide parameters for the glob-pattern plus dotenvOptions

So what is the replacement of the load function in your version?

bashleigh commented 5 years ago

ok so! Firstly I killed off the modifyConfigName method and replaced it with in object version like so

export default {
 __ name: 'this-is-my-config-name',
}

You can then use this with the InjectConfig decorator @InjectConfig('this-is-my-config-name') as well as with the config service ConfigService.get('this-is-my-config-name.something');. However! To use the inject with providers you'd need to add the prefix with either the method or manually which I wanted to avoid, so it's also possible to define your own provider

export default {
  __provide: 'this-is-my-token',
}

This won't work with ConfigService.get('this-is-my-token') because __name is a reference which is prefixed with __CONFIG__this-is-my-config-name and __provide was meant for injecting like so

{
    provide: SomeClass,
    inject: ['this-is-my-token'],
    useFactory: (config) => new SomeClass(config),
}

For your example you want to do this

@Module({
  imports: [
    ConfigModule.forRootAsync(
      Path.resolve(__dirname, "**/!(*.d).config.{ts,js}"), {
        dotenv: {
          path: path.resolve(__dirname, "../env", !env ? ".env.local.yml" : ""),
        },
      },
    ),
  ],
})

Also @types/dotenv was made a peer dependancy for when you want to use DotenvConfigOptions

I've also provided a README and a basic CHANGELOG in the next branch :) https://github.com/nestjsx/nestjs-config/tree/next

Hope this helps! Anything else, let me know!

bashleigh commented 5 years ago

Are you using classes or objects as your configs? Also make sure they're export default! Haven't figure that one out yet neither.

ghost commented 5 years ago

I am using classes in the same format you described in your Readme:

export default class TypeormConfig {
  static type: string = process.env.TYPEORM_TYPE;
  static host: string = process.env.TYPEORM_HOST;
  static username: string = process.env.TYPEORM_USERNAME;
  static password: string = process.env.TYPEORM_PASSWORD;
  static database: string = process.env.TYPEORM_DATABASE;
  static logging: boolean = JSON.parse(process.env.TYPEORM_LOGGING);
  static synchronize: boolean = JSON.parse(process.env.TYPEORM_SYNCHRONIZE);
  static port: number = parseInt(process.env.TYPEORM_PORT);
}

Regarding your second last comment, maybe you can adjust the Readme based on this information.

ghost commented 5 years ago

https://github.com/bashleigh/nestjs-config-v2-prototype#inject-via-type

In this section you added an example how to inject a class as config-object, but you wrote The below will not work as ConfigProviders are created asynchronously via the ConfigModule.. Maybe you can also add a working example below? Would be really helpful to have working examples in the Readme and not only non-working ones... ;-)

bashleigh commented 5 years ago

I did! I've been doing that though. It does work! I'll remove that! Thanks! Unless you would like to remove that part of the readme?

bashleigh commented 5 years ago

Sorry I realised that's the readme of the prototype. If you use the README from nestjs-config@next that'll be more up-to-date. 'Should' be anyway. I have added a similar snippet but I'll remove it

ghost commented 5 years ago

https://github.com/nestjsx/nestjs-config/tree/next#drawback

Why do I have to provide the constructor-invocation on my own here? I mean the whole idea behind this feature is just binding the config-objects to the di-container instead or in addition to the config-service...this shouldn't not be so hard to implement, right? I really don't like the current implementation.

bashleigh commented 5 years ago

you don't. you can use DI. I removed that section.