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

[Bug]: the async configuration example given in nestjs-module emits an error #215

Closed
opensas opened this issue Sep 10, 2020 · 3 comments · Fixed by #302
Closed

[Bug]: the async configuration example given in nestjs-module emits an error #215

opensas opened this issue Sep 10, 2020 · 3 comments · Fixed by #302

Comments

@opensas
Copy link
Contributor

opensas commented Sep 10, 2020

Bug Report

Current behavior

The following sample code

@Module({
  imports: [
    OgmaModule.forRootAsync({
      useFactory: (config: ConfigService) => ({
        service: {
          json: config.isProd(),
          stream: {
            write: (message) =>
              appendFile(config.getLogFile(), message, (err) => {
                if (err) {
                  throw err;
                }
              })
          },
          application: config.getAppName()
        },
        interceptor: {
          http: ExpressParser,
          ws: false,
          gql: false,
          rpc: false
        }
      }),
      inject: [ConfigService]
    })
  ]
})

Throws the following error:

  • write(message) should return a boolean

Expected behavior

The example should work flawlessly

Possible Solution

To fix the errors I had to do the following:

@Module({
  imports: [
    OgmaModule.forRootAsync({
      useFactory: (config: ConfigService) => ({
        service: {
          json: config.isProd(),
          stream: {
            write: (message: string): boolean => {
              appendFile(config.getLogFile(), message, (err) => {
                if (err) {
                  throw err;
                }
              })
              return true
            }
          },
          application: config.getAppName()
        },
        interceptor: {
          http: ExpressParser,
          ws: false,
          gql: false,
          rpc: false
        }
      }),
      inject: [ConfigService]
    })
  ]
})

Environment


@ogma/nestjs-module": "^0.3.0"
@ogma/platform-express": "^0.3.0

For Tooling issues:
- Node version: v12.11.1
- Platform:  Linux xps 5.0.0-1068-oem-osp1 #73-Ubuntu

Others:
using npm
@jmcdo29
Copy link
Owner

jmcdo29 commented Sep 10, 2020

Currently, the stream option is an union of NodeJS.WriteStream and NodeJS.WriteableStream that must have the property write from NodeJS.WriteableStream. According to NodeJS's docs, write returns a boolean, which is where this is coming from.

With that said, it may be a better idea to make this a custom interface that can be written as void, like so

export interface OgmaStream {
  write: (message) => void,
  hasColors?: () => boolean
}

And have a default for hasColors resulting in true instead of relying on information from the stream itself.

Shouldn't be too hard of a change, would you like to tackle it in a PR?

jmcdo29 added a commit that referenced this issue Oct 25, 2020
The log methods now accept an object instead of multiple parameters to allow for the passing of
extra keys. These keys can be anything, from cloud-provider to anything else you want to add.
There's also a lot of refactoring and making some new interfaces for cleaner passing of parameters

BREAKING CHANGE: log methods now take an object as the second parameter instead of having 3 extra
optional parameters

fix #215 #228 #297
@jmcdo29 jmcdo29 mentioned this issue Oct 25, 2020
jmcdo29 added a commit that referenced this issue Oct 25, 2020
The log methods now accept an object instead of multiple parameters to allow for the passing of
extra keys. These keys can be anything, from cloud-provider to anything else you want to add.
There's also a lot of refactoring and making some new interfaces for cleaner passing of parameters

BREAKING CHANGE: log methods now take an object as the second parameter instead of having 3 extra
optional parameters

fix #215 #228 #297
@jmcdo29
Copy link
Owner

jmcdo29 commented Oct 25, 2020

Added to v0.4.0. This will most likely be the last pre v1 version. Thanks for the idea

jmcdo29 added a commit that referenced this issue Jun 20, 2021
The log methods now accept an object instead of multiple parameters to allow for the passing of
extra keys. These keys can be anything, from cloud-provider to anything else you want to add.
There's also a lot of refactoring and making some new interfaces for cleaner passing of parameters

BREAKING CHANGE: log methods now take an object as the second parameter instead of having 3 extra
optional parameters

fix #215 #228 #297
@kuncevic
Copy link

@jmcdo29 In my case I have some other errors using Async Config:

useFactory types mismatch

TS2322: Type
(config: ConfigService<Record<string, unknown>, false>) => {   service: {     json: any;     stream: {       write: (message: any) => void;     };     application: any;   };   interceptor: {     http: typeof ExpressParser;     ws: boolean;     gql: boolean;     rpc: boolean;   }; }/

is not assignable to type

(...args: any[]) => OgmaModuleOptions | Promise<OgmaModuleOptions>

as well ass everything config.prop() does not exist, e.g TS2339: Property isProd does not exist on type ConfigService<Record<string, unknown>, false

I've pretty match copy-pasted the code additionally had to install @nestjs/config.

any ideas?

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 a pull request may close this issue.

3 participants