logger.ts undesirably monkey patches loglevel

[ghostec]

node-slack-sdk/src/logger.ts

Line 65 in 4024612

const originalFactory = log.methodFactory;

This will change the log format of projects that already use loglevel to "[INFO] undefined ..."

I had to revert the monkey patch in the file I'm importing @slack/client

const logger = require('loglevel')
const originalFactory = logger.methodFactory
const { WebClient } = require('@slack/client')
logger.methodFactory = originalFactory

[aoberoi]

@ghostec thanks for bringing this up. i can see why this becomes an issue across a project where the loglevel package is used. we are using the documented suggestion for "plugins", but maybe we need to find another technique for generating the output we desire without affecting other code. if you have any ideas about how we can do this, we would be happy to help work on that.

[clavin]

To get rid of the presence of "undefined" in the log message, it's trivial to change the method factory to only append loggerName when it's not undefined. That doesn't really hit the core issue, though.

It seems like implementing a plugin on the logger is the right path, but there's more to it. Instead of mutating the global logger, the loggers created by the getLogger export should be mutated:

interface LoggerMethod {
    (...msg: any[]): void;
}
interface LoggerMethodFactory {
   (methodName: LogLevel, logLevel: 0 | 1 | 2 | 3 | 4 | 5, loggerName: string): LoggerMethod;
}

// could do with some better naming, the idea is this function generates `LoggerMethodFactory`s
function loggerMethodFactoryFactory(name: string, originalFactory: LoggerMethodFactory): LoggerMethodFactory {
    // return a LoggerMethodFactory
    return (methodName: LogLevel, logLevel: 0 | 1 | 2 | 3 | 4 | 5, loggerName: string) => {
        // chain with the original factory
        const logMessage = originalFactory(methodName, logLevel, loggerName);

        // return a LoggerMethod
        return (...msg: any[]) => {
            const segments = [`[${methodName.toUpperCase()}]`, loggerName].concat(msg);
            // TODO: something with `name`?

            // daisy chain with the original method factory
            logMessage.apply(undefined, segments);
        };
    };
}

/**
 * INTERNAL method for getting or creating a named Logger
 */
export function getLogger(name: string): Logger {
    // get a logger id
   const instanceId = instanceCount;
   instanceCount += 1;

    // get the logger instance and patch its methodFactory
    const logger = log.getLogger(name + instanceId);
    logger.methodFactory = logMethodFatoryFactory(name, logger.methodFactory);

    return logger;
}

This makes it so no globals are mutated by @slack/client, leaving users / other libraries unaffected by the SDK's changes.

This also opens up "implement[ing] logger name prefixing", but I couldn't quite figure out what that meant since the logger's name is already included.

[aoberoi]

@clavin this is great! if you're willing to work on that patch, we can land it.

and yeah, i think the TODO for logger name prefixing is just residue from before the prefixing was implemented, feel free to remove it.