bithavoc / express-winston

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

Add per-instance equivalents of all global white/blacklists #105

Closed nwah closed 8 years ago

nwah commented 8 years ago

This PR adds support for passing any any of the whitelists/blacklists as options to expressWinston.logger() and expressWinston.errorLogger().

It leaves the global arrays intact, but I've prefixed the variable names with "global" to make things less ambiguous / avoid variable naming clashes in the code where they're used.

New options are:

expressWinston.errorLogger({
    requestWhitelist: ['foo', 'bar']
});

expressWinston.logger({
    ignoredRoutes: ['/foo', '/bar'],
    requestWhitelist: ['foo', 'bar'],
    responseWhitelist: ['foo', 'bar'],
    bodyWhitelist: [...],
    bodyBlacklist: [...]
});

At our (fairly large) company we're working on common logger that anyone can/should use in their node apps, and we'd like to base it on express-winston. But, theoretically one of those people may use express-winston as well, and could silently break our common logger by modifying these whitelists globally, which makes it a no-go for us. The changes in the PR would fix this for us.

What do you think? Thanks! —Noah

rosston commented 8 years ago

Sorry for no reply on this yet! First, thanks for the clean diff and tests!

My initial thought here was that the function-specific options should merge with the default ones rather than overwrite, but I think I've come around. If a user ever wants to combine the two, they can simply do a concat.

I'll give this a more thorough review in the next day or so, but I think it'll get merged pending some minor work.

nwah commented 8 years ago

Yep, merging was my first instinct too, but then I realized that'd make it impossible to remove some of the defaults without just modifying the shared list.

The way I ended up doing it, if you really want to merge them you can manually do something like this (even if it is a little clunky):

expressWinston.logger({
  requestWhitelist: expressWinston.requestWhitelist.concat(['some', 'others'])
});
rosston commented 8 years ago

Looks good to me. Can you just add some documentation to the readme? I think it would be good enough to simply add them to the Options section for each middleware function. After that, I'll be happy to merge this.

nwah commented 8 years ago

@rosston Cool, just updated the README to list the options.

rosston commented 8 years ago

Merged! I'm going to work on and close out #92 and then I'll push a new release.

rosston commented 8 years ago

Published in v1.3.0.