Improve the methodFactory API (stop exposing the field directly, and make it easier to use)

[pimterry]

It would be better to set the method factory with a setMethodFactory() method or similar that sets the method factory and automatically rebuilds all the methods when set, rather than just having the public property currently exposed.

This should mean that:

a) people don't need to remember to set the level after changing the method factory to apply their changes

b) the API is clearer, and there's no suggestion that people should ever be calling method factory themselves (see #80 (comment))

Note that this would be a backward compatibility breaking change: we should think about whether we can easily deprecate .methodFactory but support both for a little while, or whether that's going to be enough hassle that we should just drop it and jump to 2.0 (I suspect the latter, but the former would be nice).

[pimterry]

I've now put together a potential branch with a first run at this, which people are welcome to take a look at: https://github.com/pimterry/loglevel/compare/new-plugin-API. This would be the start of loglevel v2, breaking the plugin API, but solving all the problems here and helping to tidy up a couple of leftover notes from #81.

Usage looks like:

log.wrapLogMethods(function (logMethod, methodName, loggerName) {
  return function (message) {
    logMethod("Newsflash: " + message);
  };
});

The logMethod argument is the underlying log method that would be used normally (either the default ones loglevel uses, or the result of another wrapping method if you use multiple). methodName is the level this method is for ('error'). loggerName is the name of the logger from the new getLogger(name) API, which is what I'd be expected prefixing and similar plugins to be hooking into.

The argument to wrapLogMethods needs to be a function that takes all of these, and returns a new function that should be used for logging instead, probably wrapping the given log method.

As soon as this method is called all methods are updated immediately, so you no longer have to remember to call .setLevel() to put plugins into action.

I'm curious for the opinions of people we've talked about this elsewhere: @Wirone, @marcelboettcher, @Mr0grog. Thoughts? Particularly interested on naming (I'm not totally sold on 'wrapLogMethods', or the argument names really), and whether this would actually solve your problems.

[pimterry]

Any thoughts @Wirone, @marcelboettcher, @Mr0grog?

Would this work with what you're looking for in a plugin setup? Can anybody see any obvious problems with this?

If you do want to skim through the implementation, I'd recommend looking mostly at:

• wrapLogMethods() - https://github.com/pimterry/loglevel/compare/new-plugin-API#diff-73c0491ea03683a5afd51d5382ee5afeR181 - the external API
• buildLogMethod() - https://github.com/pimterry/loglevel/compare/new-plugin-API#diff-73c0491ea03683a5afd51d5382ee5afeR113 - called by replaceLoggingMethods to build the actual method each time
• The tests: https://github.com/pimterry/loglevel/compare/new-plugin-API#diff-0892e3ee398390dd6a8b98e61aa64daeR7

[marcelboettcher]

Thanks @pimterry! For my case this would work to avoid .setLevel() for plugin init, but I would suggest the approach Winston uses. So you could add multiple plugins which are run in sequence. The default plugin logs to the console, which is basically what loglevel does is specialized on. Then anyone could add a plugin e.g. to send the error data to a server.

What do you think about that?

[pimterry]

Oooh, thanks. You mean the add/remove transports API they provide, presumably?

It's a different model, which I find interesting. They're letting you provide a set of totally independent transports, each of which is responsible for a different way of logging, rather than wrapping the underlying log methods with a sequence of decorators. Parellel vs serial, sort-of.

Implementing that here would work fine as an option for loglevel for sending data to a server, but doesn't support the various other use cases people are coming with I think (like adding prefixes to all log messages, and similar, where you do need to extend how the built-in 'transport' works). I could include both the wrap() and the addTransport() APIs, but I don't want to if I don't need to.

It should still be possible to do server error logging with the wrapping API above (send the message in the inner function in the example code above, either before or after also logging it to the original method), and as implemented you can already have multiple plugins working in sequence.

Is there anything specific Winston's API provides that you'd like here? I think everything it can do that I'm aware of is also doable in the plugin API here, just with a slightly different model.

[marcelboettcher]

Ah, ok, didn't had those use cases in mind. For my "send to server" use case your implementation will work just fine if you add the actual log level as parameter as well.

Thanks!

[shellscape]

@pimterry this one is pretty ancient, but I'm running into an issue with moving webpack-dev-middleware to this module and using the prefix plugin. it's all good until I get into tests which instantiate multiple instances of the main export, which happen to have a log property. I believe this change would remove the requirement/limitation of the prefix to plugin to only operate on the root loglevel object and allow it to configure different loggers differently. is there any planned traction to this concept, or is it so old it's now defunct?

[pimterry]

@shellscape There's no current progress on this, no, but I'm broadly open to reviving it. It stopped simply because though it is a small annoyance, it doesn't seem like anybody was hugely passionate about it being fixed, and I think there's quite a lot of value in stability & reliability here and avoiding breaking changes where we can.

If you've got a real solid use that needs these changes though, that's a different matter, and makes this more valuable. Still, I'm not sure I fully understand your use case, and why it doesn't work with the current implementation. Could you tell me a little more about what you're trying to do, and show me the code that's not working?

[shellscape]

@pimterry you'd have to take a look at the master branch of webpack-dev-middleware, lib/log.js and the same location in webpack-dev-server. Both are using loglevel and both want to apply a plugin to loglevel (the prefix plugin). Since webpack-dev-server uses webpack-dev-middleware, and node caches modules, we end up with the same base instance being shared between the two. And well, the plugin doesn't like that, for good reason since it can't apply itself to the same instance twice with different settings. It's the lack of true different instances that create the scenario. If loglevel returned a class that we instantiated, the scenario wouldn't exist.

This is probably slightly edge-casey since the probability of this is pretty low. But these are two rather high-profile modules, so maybe the edge case could be forgiven. I'd be happy to crank out the needed changes on a fork for Node 6+, but it'd require someone on your end setting up the right babel env to compile it back down for browser use. That is to say, if you feel that direction is even right for the project.

[pimterry]

This is a related but slightly separate topic to this issue, so I've opened a separate issue here: #117

[Wirone]

@pimterry I'm sorry I did not respond to your work as I should, since initial issue was reported by me... But I did not have time to test it and later I totally forgot about this. Now I'm totally outdated on this because I didn't use loglevel for a while. I'll try to look at it, but can't promise that I'll figure out something helpful ;-)

[pimterry]

No worries @Wirone, it was years ago now 😃. If you do have any thoughts those would be interesting of course, but no pressure whatsoever, there's no need to urgently try to look into this now.

[shellscape]

@pimterry that's probably another good solution to the problem we're facing, but I think that independent instances would still prevent scenarios such as those, and probably additional situations that we haven't yet encountered or considered.

[kutuluk]

In my opinion plugin API 2.0 should look like this:

//prefix.js
function apply(loggerName, options) {
  return function(methodName) {
    return function(args) {
      args.unshift(methodName, options.text, loggerName);
    };
  };
}

export default {
  apply: apply,
  signature: "loglevel-plugin-prefix"
};

//app.js
import log from 'loglevel';
import prefix from './prefix.js';

const child = log.getLogger('child');

function print() {
  log.warn();
  child.warn('\n-----');
}

log.use(prefix, {text: 'Root'});
print();

child.use(prefix, {text: 'Child'});
print();

log.use(prefix, {text: 'Root Reuse'});
print();

child.use(prefix, {text: 'Child Reuse'});
print();

child.unuse(prefix);
print();

Output:

warn Root root
warn Root child
-----
warn Root root
warn Child child
-----
warn Root Reuse root
warn Child child
-----
warn Root Reuse root
warn Child Reuse child
-----
warn Root Reuse root
warn Root Reuse child
-----