New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transports and v2 to v3 migration #1331

Open
ofrobots opened this Issue May 25, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@ofrobots
Copy link
Contributor

ofrobots commented May 25, 2018

Hi 馃憢!

With the 3.0 release coming, I am trying to get a sense for what the expectation is for Transport authors to deal with the new release. We are getting feature requests for winston@3 support already on our transport.

Looking at the upgrade guide it seems like there are significant enough API changes that I don't see an easy way to support both winston@2 and winston@3 out of the same transport package. Is the expectation that Transport authors would be releasing a new set of packages for winston@3 (e.g. winston-funky-transport vs. winston3-funky-transport). Alternatively do we need to maintain two semver major branches concurrently on for winston@2 and one for winston@3? Both of those options sound unappetizing to me.

I can imagine some shenanigans where we try to require winston dynamically to see what the user actually has available and then expose a different version depending of the precise version? That is way too hacky, and something we can't to do in our Transport 鈥 as we are a TypeScript project and want the users to have predictable usable types.

It is possible I am either overthinking it, or perhaps I am missing something. If the project authors here have thoughts on this, it would be great to have some feedback on what the vision about the Transport migration path is.

Thanks a lot!

/cc @MylesBorins @DominicKramer

@indexzero

This comment has been minimized.

Copy link
Member

indexzero commented May 26, 2018

That's a great question. There is no silver bullet here, sorry. A TL;DR for a possible approach goes something like:

  1. Upgrade your transport to the winston@3 API.
  2. Write a wrapper to your winston@3 transport that exposes a winston@2 API, e.g.:
// winston@3 symbols
// https://github.com/winstonjs/winston#streams-objectmode-and-info-objects
const { MESSAGE, LEVEL } = require('triple-beam'); 

// The `winston@3` transport you wrote in (1) above.
const winston3Transport = require('./your-winston3-transport');

module.exports = class YourLegacyTransport {
  constructor(opts) {
    this.transport = new winston3Transport(opts);
  }

  log(level, msg, meta, callback) {
    this.transport.log(Object.assign({}, meta, { 
      level, 
      message,
      MESSAGE: message,
      LEVEL: level 
    }, callback);
  }
}
  1. Expose that as require('your-transport/legacy');
  2. Document it.
  3. Done.

Of course this would not work seamlessly, but it would easy enough to document this generically or throw in some console warnings if they are using winston@2 with your winston@3 transport, e.g.:

const Transport = require('winston-transport');

module.exports = class YourWinston3Transport extends Transport {
  log(info, callback) {
    if (arguments.length > 2) throw new Error(```
You appear to be using winston@2.x
Please use require('winston-your-transport/legacy');
```);
  }

  /* your transport code continues below */
}

Regardless of the approach it's not a silver bullet is that in winston@2 every Transport had to implement their own formatting options, nothing was shareable between them. Most transport just called common.log (which is still available in winston-compat for exactly this kind of interop) from winston, so for the 80%+ they can just invoke that and pass the resultant object to the winston@3 transport for serialization. If you defined your own formatting options however, you'll have to reimplement those as formats.

Would love a PR to the upgrade guide to explain this in detail to transport authors! 馃樃

@mattberther

This comment has been minimized.

Copy link

mattberther commented May 30, 2018

In winston-daily-rotate-file, I used a check using semver to determine which version was in use and then defined the appropriate methods depending on the result.

Base class decision: https://github.com/winstonjs/winston-daily-rotate-file/blob/293d7fe6b79408b0f10b129a1726e375e6f39821/daily-rotate-file.js#L13

Log method declaration: https://github.com/winstonjs/winston-daily-rotate-file/blob/293d7fe6b79408b0f10b129a1726e375e6f39821/daily-rotate-file.js#L114-L136

My thinking on this is that, while it somewhat complicates the transport code, it allows callers to use either winston@2 or winston@3 without any changes.

@indexzero

This comment has been minimized.

Copy link
Member

indexzero commented Jun 12, 2018

That's a very interesting technique @mattberther! Thanks for sharing.

@vongohren

This comment has been minimized.

Copy link

vongohren commented Aug 24, 2018

Is this going anywhere, particularly in regards to googleapis/nodejs-logging-winston#84

@kazazes

This comment has been minimized.

Copy link

kazazes commented Aug 24, 2018

@vongohren, see this PR, explicitly for Winston@3. Could use help on the two failing test cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment