bithavoc / express-winston

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

underscore -> lodash #107

Closed rosston closed 8 years ago

rosston commented 8 years ago

Fixes #88.

Changes:

Why lodash instead of underscore?

rosston commented 8 years ago

Other underscore/lodash functions used in index.js but left unchanged:

All comparisons made by checking the docs and source for lodash 4.6.1 and underscore 1.5.2.

floatingLomas commented 8 years ago

I don't really have any cycles to review this right now, but I wanted to say:

:+1: :+1: :+1: :+1: :+1: :+1: :+1:

rosston commented 8 years ago

No worries! I got caught up in other things, so haven't done the last bit of testing before merging. I plan to do another check to make sure lodash is API-compatible with underscore for the functions we're using.

rosston commented 8 years ago

Alright, I finally did the last bit of testing I wanted to do. I cloned the underscore repo, checked out the 1.5.2 tag, and threw lodash in the test suite. Here's the git diff:

screen shot 2016-04-23 at 11 00 12 pm

And here are the results for each of the underscore functions we previously used in index.js and test/test.js:

screen shot 2016-04-23 at 10 49 40 pm

The only failed test for _.union is this:

result = _.union(null, [1, 2, 3]);
deepEqual(result, [null, 1, 2, 3]);

...which seems like a pretty worthless piece of functionality. I also manually tested undefined, and as (sort of) expected: underscore 1.5.2 keeps undefined in the resulting array, lodash 4.11.1 drops the undefined entirely. With where we use _.union (see https://github.com/bithavoc/express-winston/blob/99b3e2d/index.js#L252-253), this difference will have no practical effect, since options.bodyWhitelist and options.bodyBlacklist fall back to the base exported versions.

So! In summary, everything is fine, and I'm merging this. 😄