Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade lodash to 4.17.2 #241

Merged
merged 6 commits into from Dec 21, 2016
Merged

upgrade lodash to 4.17.2 #241

merged 6 commits into from Dec 21, 2016

Conversation

parikhshiv
Copy link
Contributor

For #217 - upgrading to latest version of lodash (4.17.2). The following functions have changed since lodash 2.4.2 (changes are visible in https://lodash.com/docs/3.10.1):

_.all -> _.every
_.any -> _.some
_.contains -> _.includes
_.invoke -> _.invokeMap
_.pluck -> _.map
_.select -> _.filter
_.unique -> _.uniq

Also, the 'this' argument (3rd parameter) has been removed from most methods - from docs:
Removed thisArg params from most methods because they were largely unused, complicated implementations, & can be tackled with _.bind, Function#bind, or arrow functions.

In this PR, I've replaced all instances of the above functions which are out of date, and tried to address every instance of the thisArg being used (by replacing it with Function#bind) - possible that I missed some, but unlikely - I've tested a basic line graph locally to sanity check.

@narenranjit
Copy link
Contributor

@parikhshiv I'm assuming all the automated tests pass as well?

Also, if you're in the office today, come find me - I can let you know an easier way to test all graphs in the gallery.

@narenranjit
Copy link
Contributor

btw, nice work! This has been a long time coming, and i'm personally looking forward to not having to dig up ancient lodash docs every time :)

@parikhshiv
Copy link
Contributor Author

Hi @narenranjit - thanks!

As discussed, the automated tests are not currently running (older version of Jasmine), and I wasn't able to get them running.

I did use your methods to test all the graphs in the gallery (once I started a local server, I could test the example graphs built into the repo directly, which match all graphs in the showcase gallery). They all render perfectly with the lodash update, but let me know if you want to see how I'm testing.

@narenranjit
Copy link
Contributor

Looks good to me. Tracking tests at #242 ; not a requirement for this PR but we should update that before the next release.

Are the changes in #237 compatible with this PR? If not that I'd merge that first and then redo this PR to work with that.

@parikhshiv
Copy link
Contributor Author

Turns out the Jasmine tests do still work - just needed to run them in the browser. Good thing too - I had only copied over the core build of lodash 4.17.2 in our vendors folder, and so we were working with limited functions. This broke most of our tests, but replacing that core build with the full build of lodash now has all of our tests passing, so I'm going to go ahead and merge this PR.

In terms of the other issues:

As we discussed, I'm going to hold off on merging #237 for now, as it sets an additional global variable that we may not need.

I'll go ahead and update #242 - I think we may be okay with our tests - I looked into upgrades from Jasmine 1 to 2, and it looks like we've accounted for the changes in our tests.

@parikhshiv parikhshiv merged commit 46ddb54 into master Dec 21, 2016
@parikhshiv parikhshiv deleted the upgrade-lodash branch December 21, 2016 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants