No longer able to attach other transports to Logger

[sfkiwi]

In the PR on the April 11th (Winston type improvements https://github.com/coinbase/gdax-tt/pull/215) in addition to updating the winston library and using the Winston types, the ConsoleLoggerFactory function now only takes an ConsoleTransportOptions object and also wraps the returned logger instance in an object.

This both hides the logger instance with the result that you can no longer attach a new transport to the returned logger, as well as preventing a transport from being added through the options argument because the argument is now a winston.ConsoleTransportOptions.

The use case that I have been using for quite some time now is to have both console logging as well as logging to a file (done through the same logger instance). This is very useful as I can keep track of the logging output directly in the terminal but I have a version of it saved to disk as well (or if I used a different transport I could have it sent to a DB).

It is quite straightforward to add other LoggerFactory functions (for file output, DB output etc) but because winston is so flexible with all the transport types it can support, it would arguably be easier to allow other transports to be passed into the Factory function. It also means that the same logger instance can support multiple transports which is likely the intended use.

The simple fix is to just change the type of options from winston.ConsoleTransportOptions to winston.LoggerOptions. This allows the caller to pass in additional transports.

The only corner case (which is the actual case in my use case) is that you now have to create your file logger at the same time as the console logger. I prefer to name my output log files dynamically based on the output of a particular module, however that module needs to be passed a logger instance on instantiation. This is one reason for wanting the ability to attach the file transports after the initial console logger is created.

[DannyHinshaw]

~Sounds like your use case is more advanced than mine, in which case this probably won't be helpful for you... but for others that run into this new issue here has been my work around customizing the logger after this winston update:~

this only suppresses the typescript warning. logger is still not working.

import * as winston from 'winston';
import * as GTT from 'gdax-trading-toolkit';
import {Logger} from 'gdax-trading-toolkit/build/src/utils';

// Application logger
export const logger: Logger = GTT.utils.ConsoleLoggerFactory({
    level: 'debug',
    colorize: true
});

// It's still possible to use 'transports', bracket notation suppresses ts warnings
logger['transports'] = [
    new winston.transports.Console({
        colorize: 'all',
        json: false,
        timestamp: true
    }),
    new winston.transports.File({filename: 'error.log', level: 'error'}),
    // new winston.transports.File({ filename: 'debug.log', level: 'debug' })
];

[sfkiwi]

that's a neat trick for suppressing the typescript warnings and perhaps I'm misreading the updated code in ConsoleLoggerFactory but the logger instance from winston is hidden in a closure. So the object that is passed back is akin to:

function ConsoleLoggerFactory() {
  let logger = new winston.Logger()
  let returnedObj = {
     log: logger.log,
     error: logger.error
  }
  return returnedObj;
}

It's obviously not quite like this but just trying to illustrate that the winston logger instance is not actually available in the returned (gdax-tt)Logger instance.

So, to my understanding at least, you are attaching a new transport to the returned object, which would work if you were attaching it to the logger instance but I'm not sure how it would work attaching it to the returned Logger object. You would essentially end up with

{
   log: logger.log,
   error: logger.error,
   transports: winstonTransportInstance
}

[DannyHinshaw]

Well I'm glad you pointed this out, apparently my logger has not been logging to my error.log file... damn. I'll be redacting my post before as a "solution", like you said, it really only suppresses the ts warnings, apparently it's still busted.