Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

add support for custom logger configuration #117

Conversation

wingleung
Copy link

@wingleung wingleung commented May 9, 2018

Work in progress...

Warning This PR changes the logger config

example logger config to use in hypernova(config)

...
logger: {
  transports: [
    {
      Console: {
        level: 'info',
        colorize: true,
        json: false,
        prettyPrint: true,
        timestamp: true,
      },
      File: {
        filename: 'hypernova.log',
        json: false,
        prettyPrint: true,
        timestamp: true
      }
    }, 
  ],
}
...

@wingleung
Copy link
Author

wingleung commented May 9, 2018

build failed on node 6 due to npm bug, see npm/npm#20553 for more info

@ljharb
Copy link
Collaborator

ljharb commented May 10, 2018

npm bug is fixed, i've reran the errored builds.

@schleyfox
Copy link
Contributor

schleyfox commented May 10, 2018

I'm trying to do a similar thing with #104 .

The advantage to your approach is that it gets around duplication of dependencies between hypernova and the application.

The advantage to the approach in my PR is that it passes full control over the logger instance to the application embedding hypernova. The logger passed in would not even need to be a winston instance as long as it implements a log method. This comes at the cost of theoretical safety between different versions of winston.

I have a slight preference for injecting the dependency rather than passing through config, but I'm pretty indoctrinated by the Java side of the house.

EDIT: Also I know that other logging PR has been languishing, it's actually on my list to address this week.

const transportKeys = Object.keys(transportConfigs);

return transportKeys.map(transportKey =>
new winston.transports[transportKey](transportConfigs[transportKey]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this work if I have a transport installed via a separate package, e.g. https://github.com/winstonjs/winston-syslog ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this only works for core winston transports (console, file, http). trying to import winston-syslog in the app and using it in the winston instance in hypernova seems a little brittle as well, so going forward with #104 it is

@wingleung
Copy link
Author

completely agree, giving full control over a logger instance seems better than introducing a new config format to enable the limited core winston functionality. Makes it less dependent on winston as well.

@schleyfox
Copy link
Contributor

Awesome! I'm merging #104 now and will cut a new release.

Thanks for contributing!

@wingleung
Copy link
Author

closing in favor of #104

@wingleung wingleung closed this May 10, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants