Duktape reconfigures logger

[z64]

This killed logging output in our app because it reconfigures the global logger config:

https://github.com/jessedoyle/duktape.cr/blob/master/src/duktape/log.cr#L10-L15

Easily fixable by changing the order of our require in our app to load duktape first, so that we overwrite the logger config with our own. But, I am pretty sure convention is that libraries should not configure the logger - they should only set up emitters.

If duktape wishes to provide a log formatter, this should perhaps be moved to a static method for users who are not integrating duktape in to a larger app. Or just provider the formatter and leave it to users to set up themselves.

[jessedoyle]

Hi @z64! Thanks for using Duktape!

Yea, I agree with you on the point of duktape.cr globally modifying log configuration. The original intent for the provided log formatter was to be able to differentiate between console.alert() vs console.log() messages.

With that being said, I think you're absolutely correct that duktape.cr as a library shouldn't be concerned about application level log configuration.

It would be nice to retain the provided JSON log formatter as an opt-in solution (i.e. maybe via a static method call, or maybe just provide the formatter class without configuring).

I can look into this over the next few weeks, but I'm certainly open to any PRs that implement a fix for this. As you said, a simple solution is to keep Duktape::Log module definition, but just remove the Log.setup_from_env call to not override global log configuration. I'd be willing to merge a PR that does this!