replacing file-stream-rotator with winston

[lirantal]

Fixes #1285

Replacing file-stream-rotator with a better logging mechanism using winston which can be extended later for other use cases and integrations

file-stream-rotator has been deprecated, no longer maintained and right now pose a security vulnerability for the MEAN.JS project.
This PR

• adds winston as the logging mechanism which can be built upon for further and better log management in future refactoring and features.
• essentially deprecates the log options for now:

  log: {
    // logging with Morgan - https://github.com/expressjs/morgan
    // Can specify one of 'combined', 'common', 'dev', 'short', 'tiny'
    format: 'dev',
    options: {
      // Stream defaults to process.stdout
      // Uncomment/comment to toggle the logging to a log on the file system
      // stream: {
      //  directoryPath: process.cwd(),
      //  fileName: 'access.log',
      //  rotatingLogs: { // for more info on rotating logs - https://github.com/holidayextras/file-stream-rotator#usage
      //    active: false, // activate to use rotating logs
      //    fileName: 'access-%DATE%.log', // if rotating logs are active, this fileName setting will be used
      //    frequency: 'daily',
      //    verbose: false
      //  }
      // }
    }
  },

It is possible to preserve some of the settings but maybe we need to change naming conventions here as they were strictly related to the morgan HTTP requests logging and with winston we will be providing more than that.

P.S.
Right now tests fail because there are checks for that specific log configuration option so I can either fix it with some adjustments and keep some of the options or remove the tests for this fix to be added now due to the file-stream-rotator vulnerability.

[lirantal]

Many tests fail here which seem unrelated to the change in this PR.
Any ideas?

[lirantal]

When refactoring the logger requires refactoring the tests too:

[coveralls]

Coverage decreased (-0.1%) to 70.51% when pulling 1f75f9f on lirantal:feature/add-winston-logger-support into 5da5a61 on meanjs:master.

[lirantal]

@meanjs/contributors things have been refactored and accommodated. Any comments?

[mleanos]

On initial review, this looks good. I'd like to pull it down and test sometime over the next couple of days.

The refactor seems good too. I think I had originally built this out too tightly coupled with the file-stream-rotator library. Thus, the tests ended up being too specific, which more than likely created a bit more work for this refactor.

Thanks @lirantal! I like this implementation much better.

[lirantal]

Thanks @mleanos
I like it too :) specifically because we're now using winston which is a more solid library and this PR also integrates morgan with it so you get to win from both general app logging and express's HTTP requests logging and can consolidate them however you want. Furthermore, because winston allows to plug any other transports like logging to cloud services, network, etc then this is easily extendable by anyone.

After this one is merged my next PR on this topic will be to clean up all the console.log() and instead use the logger facility which is now available.

[mleanos]

@lirantal I've tested this, and it seems to be working as expected.

Can we turn the console logs off during the tests? Having the "info" logs in the console makes it a bit harder to follow the tests, and is a bit distracting. Previously, the logger would only use the file system log when the tests were ran; I think this might be the desired behavior by most devs.

[lirantal]

Hi @mleanos,
I totally agree and updated the PR with those 2 suggestions.

[coveralls]

Coverage decreased (-0.4%) to 70.21% when pulling 5b9b2fd on lirantal:feature/add-winston-logger-support into 5da5a61 on meanjs:master.

[lirantal]

@mleanos don't want to rush you but I do have a few other topics I'd like to fix which are infrastructure wise and I'd hate to deal with all the conflicts resolution later.

[mleanos]

@lirantal Thank you! I just pulled this down, and the "info" logs are still being logged to the console.

I think we just need to comment out this line: https://github.com/lirantal/meanjs/blob/5b9b2fdee63c90f97864f8cbb9d131ab455abd8b/config/env/test.js#L18

Other than that, this all looks good to me. I commented out the above referenced line, and the console messages went away. Feel free to merge if you agree with my suggestion.

Also, was your intention to log to the file system when the tests were ran? Just clarifying since you said two suggestions.

[lirantal]

Oh yeah, accidentally forgot to comment that out on the test.js file after I added the express config to check it. Commiting in a sec to fix that.
And no, I don't think it makes much sense to log anywhere during tests running, unless you want to debug it personally so you end up enabling it and that's fine.

[coveralls]

Coverage decreased (-0.6%) to 69.989% when pulling 6e2721c on lirantal:feature/add-winston-logger-support into 5da5a61 on meanjs:master.

[lirantal]

I'm gonna merge this one even though we got a low coverage score, at least we'll be removing the vulnerable and un-maintained file-stream-rotator package already, and we can take care of coverage later on.