WIP: Multiple logger support

[Mr0grog]

This is a simplistic implementation of #68 with minimal changes to the actual codebase (it looks a bit bigger than it really is because many methods moved into the logger constructor).

For the most part, all this ultimately does is add a method named getLogger(name) to the library. That method returns a new logger with the given name, or, if it’s already been called with that name, returns the previously created logger. The library itself is actually just an instance of one of these loggers, but it has the getLogger() and noConflict() methods sprinkled on top.

There are a couple ugly things in here, mostly pertaining to the discussion of inheritance: https://github.com/pimterry/loglevel/issues/68#issuecomment-120677667

• There’s a private variable named defaultLoggerLevel that tracks the current level of the default logger so it can be passed to new loggers. This would be better handled if logger instances had a getLevel() method, which would also solve #70 / fit with #77
• There’s a little nuttiness with some private methods needing to be called with .call(). This is really because methodFactory needs to not be tightly bound to an instance in order for it to be passed along to new loggers. Maybe all private methods should be this way? Maybe some more rewriting would be in order, or a changed signature for methodFactory, or something related to having setters for methodFacotry (as noted in https://github.com/pimterry/loglevel/issues/70#issuecomment-121411810) would help. Haven’t thought too deeply here yet.

[pimterry]

Great work, thanks for chasing this! I think overall this approach is very good. I've put a couple of comments inline on specific tweaks, and there's a couple of wider things:

• This needs rebasing, as I've now merged the getLevel() PR.
• Needs documentation.

• I do think the code would be much nicer if we didn't need the .call(this) stuff. I don't have an easy answer I'm afraid. Moving the logger parameter from being a this that we have to remember to instead being an explicit argument would be an easy improvement on the enableLoggingWhenConsoleArrives and replaceLoggingMethods methods though.

  The bit where that becomes tricky is the methodFactory method itself, which I'm not as sure how to handle. Maybe that should just take a logger argument too, but that's not something most plugins should need to implement a method factory, and I'm worried it'll encourage to people build plugins badly (e.g. checking logger.getLevel() to decide whether the log factory should be building a method, rather than just leaving that to the logger and focusing on returning a new method). Maybe that's not something to be concerned about though. I don't have any easy answers there, any more thoughts you had would be great.

• Method factory more generally: I am currently planning to move to a setMethodFactory() API very soon. I'm either going to deprecate the previous approach and support both for a little while, or just bite the bullet and release all this as v2.0 instead. Just FYI, in case that helps your approach to the previous point.

[Mr0grog]

Ok, I spent a little time this afternoon cleaning up the tests, adding docs, and fixing methodFactory so it gets the right args (forgot about that last time around).

I haven’t worked on better solutions to the private-but-shared-across-instances issue with enableLoggingWhenConsoleArrives et al. However, some thoughts:

Maybe that should just take a logger argument too, but that's not something most plugins should need to implement a method factory

I’m skeptical that a third-party plug-in shouldn’t be capable of doing something the built-in one can. Besides, the current implementation already sends methodFactory the logger—just as this rather than an argument.

That said, I get the idea that the API should hopefully push developers towards doing the “right” thing. Maybe one approach would be to send the methodFactory a function that resets the logging methods (i.e. a version of replaceLoggingMethods that takes no arguments) and not be given the actual logger instance as this. That severely constrains what a plug-in can do, while still enabling current functionality. But maybe all this is better discussed in relation to v2 or adding setMethodFactory(). Is there an issue for that?

[pimterry]

I’m skeptical that a third-party plug-in shouldn’t be capable of doing something the built-in one can. Besides, the current implementation already sends methodFactory the logger—just as this rather than an argument. That said, I get the idea that the API should hopefully push developers towards doing the “right” thing.

Mm, my concern isn't really so much what's possible, but what the API encourages people to do. If you only need the logger in strange and wonderful cases, I'd rather not pass it as an explicit argument. Providing it only as the context feels less visible and suggestive, although I realise that's a pretty fluffy argument.

But maybe all this is better discussed in relation to v2 or adding setMethodFactory(). Is there an issue for that?

There is now: #82.

I was thinking this wouldn't make much difference, but actually I think it really does. A big part of this current problem is that the current method factory API is quite unfriendly to this, and we're exposing the default factory, so we have to be careful to make that usable. If we're breaking the API anyway we can stop doing both of those though, so that writing a plugin looks more like:

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

No need to read the current log factory (which is good, because that field being exposed is part of why I want to change this), and nobody ever sees the default method factory at all, only its output.

That's all great for this, as it turns out, because that means we can split the internal representation. Rather than just having a log.methodFactory property for everything, we can have a private methodFactory function inside the Logger (lexically inside, so it can closure in self like every other private function), plus a separate customMethodWrapper field, which is undefined unless set with .wrapLogMethods, and which can be safely copied between instances.

Some of those details are still a bit vague, but I'm reasonably confident that this means we can clean this up easily once that's added. Given that, lets just ignore this for now.

Other than things the next change will fix I think this looks great, so I'm going to merge it now and then get started on the improved plugin API. Any thoughts you have on all this would be very interesting though, over on #82.

Thanks!

[davetayls]

looking forward to this one :+1: