Simplify the logger facade
Closed this issue · 9 comments
The logger facade is clean but a bit complex.
Maybe a simpler facade would be cleaner, e.g
var winston;
try {
winston = require('winston');
}
catch(e) {}
if (!process.env.DEBUG) {
global.Logger = {
log: function() {},
info: function() {},
error: function() {},
warn: function() {},
};
} else if (winston) {
global.Logger = winston;
} else {
global.Logger = console;
}Well, the thing is, which I really don't like about some modules out there is inflexible logging (user is forced to dig out the code to set it up or it just works with console or specific logger.)
I've designed it like to actually be able to use any logger (e.g. bunyan), nut just winston, or none at all if you wish so. The facade is meant to keep the code simple in other parts of the lib that do logging.
Your suggestion is good but it makes a dependency on winston or console, adds to global space and I really don't think using process.env.DEBUG is flexible enough because it makes assumptions the client would designate a non-prod environment (or a environment with different logging) like that, when in reality it can be numerous other ways. So I believe it is most flexible for client to handle this logic and give the logger (or not) to options argument as he/She sees fit, based on their situation in their code that would use this lib.
What do you think? @jkernech
There is some complexity when you want to actually log to debug without ENV vars because in case of containers, you sometime don't want to modify code and recompile/rebuild to check the logs. So adding an ENV var and relaunching the container is convenient, maybe we could add that and fallback to console if no logger is available and env var is set
The facade simplify the logging and abstract the logger, so winston should be removed of the peerDependencies too if we want to keep it this way
About the complexity of the code, we have now 3 files to cover and maintain but the lib is not related to logging so there is some % of coverage related to something the lib is supposed to handle. Would it be explodable in another module?
@mrister You have a point and I agree with you. Since it's a library the client should handle the logic of integration in order to have the capability to use his own logger and/or adapt the logging to his environment. We want to keep the library as readable and clean as possible so the user won't have to dig in the code or be force to use a specific logging library.
But @PuKoren also have a point, the module is not related to logging so we shouldn't have to handle this kind of logic in a library otherwise we will have to maintain code and tests for each library we manage..
What do you think of creating another module for the logging facade ?
@PuKoren winston is a peerDependency and does not need to be installed with this lib. I do use it in tests. True, the module is not about logging, but has logging requirements. Yes, there are 3 small files dedicated to logging and could be extracted to another module but I think they are a bit too specific because of our logging guidelines (log lines like '[module:name:submodule-name: function-name] message). As another lib I would expect it to be more flexible here and have more dedicated tests which means more work invested into it. Now taking that into account I really don't mind working on a supporting lib like this, but do you guys ?
I believe it should work normally already for any logger that is out there and has log, debug, info, error,.. functions.
If @PuKoren and @jkernech or anybody have any other good ideas on how to reshape this, i'd be glad to hear and implement them :-) Maybe somewhere on another project ?
I designed it like this so these are separate testable and reusable components but I wonder if we would be even discussing this if it were more 'poorly' designed and each model in the lib had their own way of handling logging with constant if then else checks on logger and it's functions. Maybe it wouldn't pop up so clearly like it does now :-)
Anyway, whichever route you guys suggest (refactor, another lib) is fine, just let me know :-)
What you have done is clean, concise and tidy, and since it's already done and tested I'm fine with it. I think the only fear here was to see grow a part of the lib that goes beyond the requirements
I would vote to keep it and see how it goes, if someday we notice that we copy/paste the helpers into other libs because it's useful we may consider to move it
About the logger:
I think it would be nice to be able to enable/disable the logging using env vars (to run debug or tests sessions without changing the code/config if we want to check if the errors on the host app comes from one of the providers or the lib)
About peerDependency: if it is used on tests and the use of the logger is generic (if we want to use winston, pass it to the lib) then we can set it as dev dependency so it don't lead to misunderstanding (I don't know for you guys but when I see a peerDependency I expect the lib to behave differently if I satisfy the dependency or not, without changing the lib inclusion options)
Yeah I would be ok with this approach also and yeah and moving winston to devDep too. Will do that.
Errors are basically bubbled up to user of the lib via promises so only enabled debugging would help by giving more info.
I would rather have the client explicitly say something like:
phoneNumberUtils.init({logger : process.env.DEBUG ? winston : null });Then us assuming this is what he wanted just by setting process.env.DEBUG, but if you guys insist on that, then ok, I will implement it. It's ok and does make sense. It's just a matter of flexibility per my way of thinking I guess.
Ok that's fine for me