joemfb / ml-search-ng

angular module for MarkLogic search applications
https://joemfb.github.io/ml-search-ng/
9 stars 10 forks source link

Replace deprecated lodash functions #110

Closed patrickmcelwee closed 7 years ago

patrickmcelwee commented 7 years ago

See https://github.com/lodash/lodash/wiki/Deprecations

I discovered this when trying to "See more" facets on a project using Lodash 4.x and got an error (_.where is not a function). I believe these lodash changes are backward-compatible (they just removed one of the aliases)

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 141394d3abdf6dd474c3dec02dfeb63bb1ee539e on patrickmcelwee:lodash-deprecations into 7309ed522670d5b54c8daa4edccc8c13fa8bb5ef on joemfb:master.

joemfb commented 7 years ago

The tests all passed with lodash 3, so it's definitely backwards compatible. Do you want to update this PR to include the lodash version change in bower.json? The tests should run again automatically.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-1.07%) to 98.93% when pulling d8cab0930fa65ef17faac77c0f15938d61cdf2a2 on patrickmcelwee:lodash-deprecations into 7309ed522670d5b54c8daa4edccc8c13fa8bb5ef on joemfb:master.

patrickmcelwee commented 7 years ago

lodash 4.x also got rid of the optional thisArg to .each

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling c261bfb92f47e9f2b0833ca2a6fcf76ee65feeb4 on patrickmcelwee:lodash-deprecations into 7309ed522670d5b54c8daa4edccc8c13fa8bb5ef on joemfb:master.

patrickmcelwee commented 7 years ago

by the way, I noticed that the dist js files have the old version number (0.2.6) ... not sure it matters much, but I kept that out as part of this PR (gulp was auto-updating it to 0.2.7) ... but probably should be updated, right?

patrickmcelwee commented 7 years ago

Should we have as permissive a lodash versioning as possible? > 2.whatever and < 5? - I did verify locally that the _.bind code works on the older lodash version

joemfb commented 7 years ago

The dist/ files should just be ignored. I commit updates to them when I cut new releases, and they're not used by the tests.

Permission versioning is probably best. I just wish there was a way to make the unit test try all the versions ...

patrickmcelwee commented 7 years ago

ok, I bit ... here's a bash script that I used to confirm that versions 3.0.1 to the most recent work against the tests (3.0.0 and 4.0.1 failed, presumably due to lodash bugs):

# /bin/bash
function version_gt() { test "$(printf '%s\n' "$@" | gsort -V | head -n 2)" != "$1"; }

versioninfo=$(bower info lodash | grep '-' | tr -s ' ' | cut -d ' ' -f 3)
for version in $versioninfo; do
  if version_gt $version '1.99999.999999'; then
    bower install lodash#$version | grep "install lodash";
    gulp test | grep "FAILED"
  fi
done
patrickmcelwee commented 7 years ago

2.4.2 fails, though, because _.includes was not a function then. So >= 3.0.1 < 5 ?

patrickmcelwee commented 7 years ago

I think this should be ready to merge now ... great that you already have that test coverage!

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 42a158f813440e2e7b08313b88624fd418a0b8ac on patrickmcelwee:lodash-deprecations into 7309ed522670d5b54c8daa4edccc8c13fa8bb5ef on joemfb:master.

coveralls commented 7 years ago

Coverage Status

Coverage remained the same at 100.0% when pulling 42a158f813440e2e7b08313b88624fd418a0b8ac on patrickmcelwee:lodash-deprecations into 7309ed522670d5b54c8daa4edccc8c13fa8bb5ef on joemfb:master.

grtjn commented 7 years ago

+1 on still allowing 3.x. Not sure lodash 4 works with node 0.10, which is what slush still runs on (is tested for)..

joemfb commented 7 years ago

Thanks for working through this, @patrickmcelwee! Sorry for the delayed merge ...

@grtjn, this is just the bower dep, it shouldn't be coupled to the node version in any way.

grtjn commented 7 years ago

I was talking about slush-ml-node. Then again this is bower, not npm. So just ignore me.. 😉