Support skipping logger creation if it is provided#6
Support skipping logger creation if it is provided#6potatopankakes wants to merge 1 commit intogodaddy:mainfrom
Conversation
msluther
left a comment
There was a problem hiding this comment.
Seems like a fair enough feature.
The docs could use some updating though to reflect this.
e.g. https://github.com/godaddy/slay#app , https://github.com/godaddy/slay#app-startup-in-detail
And perhaps a sample of how to provide your own logger and what that "interface" for a logger should look like (given that some internal components to slay are going to call methods on app.log).
Also perhaps an e2e test showing that a custom logger can work here without crashing, stuff actually gets logged to it, etc.
|
|
||
| require('godaddy-test-tools')(gulp, { | ||
| lint: { files: ['*.js', 'lib/**/*.js', 'test/*.js'] }, | ||
| lint: { files: ['*.js', 'lib/**/*.js', 'test/**/*.js'] }, |
There was a problem hiding this comment.
I'm not super familiar with gulp, but depending on platform and how this gets used you may need to include both test/*.js and test/**/*.js.
indexzero
left a comment
There was a problem hiding this comment.
Reasonable enough indeed, but we should probably put in the effort to discuss the capabilities of such a module an document them in-depth before saying "any object will do for app.log".
A good way to start that discussion is: what logger are you replacing winston with? We can do a capabilities diff from there as a straw person.
| * a winston Logger for this app instance. | ||
| */ | ||
| module.exports = function (app, options, callback) { | ||
| if (app.log) { |
There was a problem hiding this comment.
This capability checking is not quite enough as there are a few calls to app.log.info in the other preboots.
The allows:
winstonlogger.winstontransport configuration, when not used.