airbnb / hypernova

A service for server-side rendering your JavaScript views
MIT License
5.82k stars 207 forks source link

add support for custom logger configuration #117

Closed wingleung closed 6 years ago

wingleung commented 6 years ago

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 commented 6 years ago

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

ljharb commented 6 years ago

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

schleyfox commented 6 years ago

I'm trying to do a similar thing with https://github.com/airbnb/hypernova/pull/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.

wingleung commented 6 years ago

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 commented 6 years ago

Awesome! I'm merging https://github.com/airbnb/hypernova/pull/104 now and will cut a new release.

Thanks for contributing!

wingleung commented 6 years ago

closing in favor of https://github.com/airbnb/hypernova/pull/104