Upgrade to lodash@4.17.11 #240
Conversation
|
1 test broken, could you fix it? |
lib/mincer/asset_attributes.js
Outdated
| @@ -150,9 +150,9 @@ getter(AssetAttributes.prototype, 'extensions', function () { | |||
| * // -> ".js" | |||
| **/ | |||
| getter(AssetAttributes.prototype, 'formatExtension', function () { | |||
| return _.find(this.extensions.reverse(), function (ext) { | |||
| return _.find(this.extensions.reverse(), _.bind(function (ext) { | |||
There was a problem hiding this comment.
What are reasons to use lodash's _.bind() instead of native .bind()?
There was a problem hiding this comment.
The reason for that, is because I followed the details in the Lodash change log, for Lodash 3 to 4 compatibility. which suggests this way https://github.com/lodash/lodash/wiki/Changelog#compatibility-warnings
There was a problem hiding this comment.
_.bind() cares about special cases like Object, but we have only functions here. I think native .bind() will be ok, and less messy.
|
@puzrin thanks for initial feedback, I will look into the test |
|
@puzrin When running the tests on the master branch, they pass on Node 6, but Node 4 fails, which is the same case as this branch. So I'm not convinced they are related to this branch, unless you can confirm otherwise? Or perhaps there is some extra configuration needed which my environment doesn't have |
|
https://travis-ci.org/nodeca/mincer/builds/497372081 As far as i see, "ping" fails everywhere, before this PR too. Other tests - depends on node version. I'd suggest to prepare release v2.0.0, and drop all old nodes there (if you don't need those in your |
Lock uglify-js at version 2, as version 3 introduced breaking changes
|
@puzrin I have addressed the things discussed above, as well as the server ping test failing on master (this was due to some breaking changes in latest releases of |
|
Hm. Lets split to different PR-s, for clear?
|
|
Or just create a new PR with lodash-related fix only, and i'll commit the rest. Also, let me know what minimal node version do you need? Is 6.0 still required or we can start from 8.0? |
|
@puzrin thanks. This PR is now the lodash 3 to 4 upgrade only, which includes the I think that it would be good to keep it at Node 6 as a minimum, as we have a passing build with that version |
|
@puzrin thanks for expediting this - could you please publish 2.0.0 to npm? |
|
done |
* Upgrade to lodash@4.17.11 (js-kyle#240) Close js-kyle#239 * Lock devDependencies before breaking change versions * Deps: drop ndoc due problems with install * Travis: drop node v4, add latest * 2.0.0 released * Revert "Deps: drop ndoc due problems with install" (highlight.js fixed) This reverts commit 3139dcd. * security: lodash@4.17.14 * Travis-CI: update minimal node version to 8 * Dev deps bump & CS update * Added deprecation warning * 2.0.1 released * nodeca = js-kyle * update Makefile
#239
Would like to get this through, so I can then upgrade the mincer version of
connect-assetswhich relies on mincer heavily (security updates for Lodash)Edit: please let me know about the build status - there is a test failing on the Node.js 6 which also fails locally. Not sure what is going on with Node 4, and whether or not it is related