Bind the custom logger to the log methods, useful for loggers that use "this" on their own log methods #25
Conversation
…e "this" on their own log methods
|
Probably related to balderdashy/sails#4337 |
|
Created a project that uses winston 3.2.1, where the logger is working correctly with the current implementation https://github.com/luislobo/sails-test-2019 Check https://github.com/luislobo/sails-test-2019/blob/master/config/log.js |
There was a problem hiding this comment.
@luislobo thank you! Added a couple of notes about .bind() for the fallback log method, and also about falling back to a no-op log method in a couple of cases
index.js
Outdated
| logger.debug = logger.debug ? logger.debug.bind(logger) : nullLog; | ||
| logger.info = logger.info ? logger.info.bind(logger) : nullLog; | ||
| logger.error = logger.error ? logger.error.bind(logger) : nullLog; | ||
| logger.warn = logger.warn ? logger.warn.bind(logger) : logger.error; |
There was a problem hiding this comment.
looks like we kept the "error" fallback, but lost the nullLog short-circuit here
There was a problem hiding this comment.
Also missing a .bind right? so like logger.warn? logger.warn.bind(logger) : logger.error? logger.error.bind(logger) : nullLog ?
There was a problem hiding this comment.
In the warn method? well at the time warn is being set, error should be whether the error function if it exists, or the nullLog assigned in the error shortcut, am I correct?
There was a problem hiding this comment.
@mikermcneil In case you didn't see my comments up there
There was a problem hiding this comment.
@mikermcneil I see what you say, I've added the bindings
index.js
Outdated
| logger.info = logger.info ? logger.info.bind(logger) : nullLog; | ||
| logger.error = logger.error ? logger.error.bind(logger) : nullLog; | ||
| logger.warn = logger.warn ? logger.warn.bind(logger) : logger.error; | ||
| logger.crit = logger.crit ? logger.crit.bind(logger) : logger.error; |
There was a problem hiding this comment.
same here- kept the "error" fallback but lost the nullLog short-circuit
There was a problem hiding this comment.
also, I think we're losing the bind in our fallback -- maybe it should be logger.crit? logger.crit.bind(logger) : logger.error? logger.error.bind(logger) : nullLog ?
There was a problem hiding this comment.
@mikermcneil I see what you say, I've added the bindings
|
Anything I can do to help move this PR along? Winston is preferred logging mechanism, and without this PR the latest version of winston not working. I want to use winston so I can leverage sentry.io |
|
@luislobo Are you able to tie this off? It would mean so much to a lot of people :) |
|
@mikermcneil This should be ready to review/merge. All others, sorry about the delay, as always, open source development time is a luxury only a few can do full time ;) |
|
@mikermcneil Are you able to look at this/merge it? |
Bind the custom logger to the log methods, useful for loggers that use "this" on their own log methods
While using the latest winston 3.2.1, issues arise throwing error
self._addDefaultMeta is not a functionas reported in balderdashy/sails#4601This fixes the issue