search

TomFrost / Bristol

Insanely configurable logging for Node.js
MIT License
113 stars 19 forks source link

Move standard formatters to public export #34

Open rooftopsparrow opened 7 years ago

rooftopsparrow commented 7 years ago

Hi @TomFrost and contributors,

Would you be open to moving including the formatters included with the module the top level export ? One example for the reasoning behind this request:

The json formatter is almost 100% what I want for everything, but I'd like to add static properties to a particular target ( not globally ). To achieve that, I can modify the function arguments before invoking the standard json function.

I currently can do this just by just reaching into the module ( require('bristol/lib/formatters/json') ) but its brittle because you can change that path an any time 😄

Thanks for the module 👍

TomFrost commented 7 years ago

Hey Jonathan! Killer suggestion.

Bristol has a stated goal of lazy-loading everything possible, so that leaves us with two options toward that goal:

  1. Define a getter. Either a literal ES6 getter where accessing Bristol.formatters.human would cause that formatter to be require()d in real time, or just a function call like Bristol.getFormatter('human').

  2. Consider the paths of formatters and targets to be versioned entities, so if the path ever needs to change, it will cause a major version change.

I like 2 for its simplicity, but 1 gives you that peace-of-mind guarantee since it's in the code. Any thoughts? :)

Also: Perhaps a better option here would be to make it possible to set "globals" just on one particular target? I'm working on a revamp of how targets are stored and configured, so this could be a very easy feature add if there aren't a bunch of other use cases where exposing the built-in formatters is still the better option.

MarkHerhold commented 7 years ago

I also modified the JSON formatter for my use and I created PR #22 so I didn't have to duplicate the Bristol helper code. I then just created my own JSON formatter based on Bristol's and used the original Bristol utils.

This worked out well for me and I don't have to rely on paths and I don't have to monkey patch JSON formatter code.

rooftopsparrow commented 7 years ago

Hi @TomFrost and @MarkHerhold 👋,

I don't mind requiring the formatter directly, and its pretty standard practice that I see around the community. I'm totally ok with option 2 if you would consider those to be versioned paths 👍

As far as "globals" per targets are concerned -- I think that would be a great feature because most of the time one log format requires some static attributes ( Looking at you Logstash ) than other simpler targets where those globals are just noise.