bithavoc / express-winston

express.js middleware for winstonjs
https://www.npmjs.com/package/express-winston
MIT License
797 stars 187 forks source link

Wrong order in example #169

Closed lfundaro closed 6 years ago

lfundaro commented 6 years ago

Problem statement

The example in the Readme has a slight order problem. Basically we add a non-blacklisted route (/) on app before we set up request logging, hence, you never see requests to / being logged by expressWinston.

How to reproduce ?

Run the following example

var express = require('express');
    var expressWinston = require('express-winston');
    var winston = require('winston'); // for transports.Console
    var app = module.exports = express();

    // app.use(express.bodyParser());
    // app.use(express.methodOverride());

    // Let's make our express `Router` first.
    var router = express.Router();
    router.get('/error', function(req, res, next) {
      // here we cause an error in the pipeline so we see express-winston in action.
      return next(new Error("This is an error and it should be logged to the console"));
    });

    app.get('/', function(req, res, next) {
        res.write('This is a normal request, it should be logged to the console too');
        res.end();
      });

    // express-winston logger makes sense BEFORE the router.
    app.use(expressWinston.logger({
      transports: [
        new winston.transports.Console({
          json: true,
          colorize: true
        })
      ]
    }));

    // Now we can tell the app to use our routing code:
    app.use(router);

    // express-winston errorLogger makes sense AFTER the router.
    app.use(expressWinston.errorLogger({
      transports: [
        new winston.transports.Console({
          json: true,
          colorize: true
        })
      ]
    }));

    // Optionally you can include your custom error handler after the logging.
    // app.use(express.errorLogger({
    //   dumpExceptions: true,
    //   showStack: true
    // }));

    app.listen(8080, function(){
      console.log("express-winston demo listening on port %d in %s mode", this.address().port, app.settings.env);
    });

had to comment a couple of things because of some complaints of express modules being deprecated etc.

When doing curl http://localhost:8080/ you will get no logs from expressWinston.

Fix:

simply put the

app.get('/', function..... })`

after

    app.use(expressWinston.logger({
      transports: [
        new winston.transports.Console({
          json: true,
          colorize: true
        })
      ]
    }));
lfundaro commented 6 years ago

please see https://github.com/bithavoc/express-winston/pull/168

makrandgupta commented 6 years ago

The order is the problem here. Instead of app.get it should have been router.get. I have tested this and will request this change in #168