bithavoc / express-winston

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

Support for Winston 3.x #184

Closed rosston closed 6 years ago

rosston commented 6 years ago

Fixes https://github.com/bithavoc/express-winston/issues/163.

Proposed changes

Anything else that really needs to get into the major version bump? Non-breaking changes (almost all of them) should wait until after the major version bump. I don't want this major version bump to have too much scope creep.

dmichelin commented 6 years ago

As per https://github.com/sindresorhus/ama/issues/479#issuecomment-310661514 It doesn't seem like having a package lock is a good idea. Given that this is a library and largely unopinionated adding a package-lock.json could introduce bloat.

bithavoc commented 6 years ago

@rosston I think shalk and lodash should be peer dependencies.

bithavoc commented 6 years ago

agree with @dmichelin It shouldn't need package-lock.

rosston commented 6 years ago

@bithavoc From looking at the original intent behind peerDependencies on the npm blog and the way they're described in npm's docs, I don't think chalk and lodash should be peer dependencies. They are packages that express-winston depends on but that the end user of express-winston doesn't need to know anything about. I believe peerDependencies should only be used for plugin-type situations.

Admittedly, it's a little bit of a stretch to think of express-winston as a plugin for winston (the one peer dependency we do have), but I think the usage kind of makes sense there if you squint at it just right. 🙂

rosston commented 6 years ago

Alright, I think this is ready for a review (mostly looking at you, @bithavoc 🙂).

The number of changes that prettier + standard introduces is annoying, but I also tried running prettier + eslint (with the eslint:recommended config) and got a similarly large number of changes. I don't like the amount of changes in either version, but I'd rather pay the price of all the changes once than semi-arbitrarily tweak a linter/formatter config. I'm open to opinions/thoughts here though.

rosston commented 6 years ago

@bithavoc I just pulled the styling changes out. You're right, it makes sense to handle/review those separately/later.

bithavoc commented 6 years ago

looks good @rosston can you also bump the version? Thanks.

rosston commented 6 years ago

Yep, I'll bump the version and update the changelog before release. I'll get this out in the next day or so, I think.

bithavoc commented 6 years ago

awesome nicely done @rosston I'd be great to add an explanation to the Readme that v2.6.0 is the last version to support express-winston@2, etc.

YasharF commented 6 years ago

@rosston Thank you for the great work!

bithavoc commented 6 years ago

@rosston we're ready to merge this, right?

rosston commented 6 years ago

Just published this as v3.0.0.