search

TomFrost / Bristol

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

Use json-stringify-safe for json formatter #35

Closed rooftopsparrow closed 6 years ago

rooftopsparrow commented 7 years ago

Hello @TomFrost and contributors,

Wondering if you'd be open to taking on another dependency and changing the default json formatter to use json-stringify-safe to protect crashing the process when trying to stringify a circular structure?

I'm willing to make a PR if this is something you'd like.

Thanks 👍

TomFrost commented 7 years ago

Another good suggestion!

My only immediate concern about this would be serialization speed. I'd love to see a benchmark of JSON.stringify versus that lib-- if you wanted to convert Bristol and test that, I wouldn't be opposed!

rooftopsparrow commented 7 years ago

Awesome!

Since there are no benchmarking tests in Bristol right now, I'll make a PR adding some initial benchmarks to establish a baseline of performance. However, I have no clue what I'm doing in terms of benchmarking, so it'd be awesome to get some feedback on things you'd like to see benchmarked.

MarkHerhold commented 7 years ago

@TomFrost json-stringify-safe is infinitely faster on circular references ;)

yknx4 commented 7 years ago

PR #41