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 to lodash 4 #3315

Merged
merged 3 commits into from
May 13, 2016
Merged

Upgrade to lodash 4 #3315

merged 3 commits into from
May 13, 2016

Conversation

forivall
Copy link
Contributor

@forivall forivall commented Feb 3, 2016

Briefly discussed on slack.

One of the test fixtures was also caught by my grep/sed'ing, just to note. (I did double- & triple-check everything after the sed & grep, ran tests, etc.) I'll also check the coverage results to make sure all the changes were covered.


export default function (dest?: Object, src?: Object): ?Object {
if (!dest || !src) return;

return merge(dest, src, function (a, b) {
return mergeWith(dest, src, function (a, b) {
if (b && Array.isArray(a)) {
let newArray = b.slice(0);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rest of this could also be replaced with _.union

@hzoo hzoo added the PR: Internal 🏠 A type of pull request used for our changelog categories label Feb 4, 2016
@forivall
Copy link
Contributor Author

It seems like there's some caching issues with the CI. I thought it might be an issue with npm v2, but I just ran the tests locally with node 3 and npm 2, and it works fine. Can the cache be cleaned somehow for this PR?

@forivall
Copy link
Contributor Author

forivall commented Mar 9, 2016

All of the tests pass on travis when running cleanly on my branch: https://travis-ci.org/forivall/babel

kcheck / eslint depends on lodash 3, and it seems to be cached in some deduplicated state. When it is restored from the cache, it doesn't specifically contain lodash, since it expecs it in the parent. But when bootstrap npm install is run, it changes the lodash version to 4, which breaks imports.

@forivall
Copy link
Contributor Author

forivall commented Apr 1, 2016

roughly reproducable via git checkout 60d773f && npm i && npm dedupe && git checkout lodash-v4 && make bootstrap && npm test

@billyjanitsch
Copy link

billyjanitsch commented May 4, 2016

Hi @hzoo and @loganfsmyth. I'd love to see this Lodash bump get merged -- without it, projects that depend on Babel and lodash@4 can't dedupe a second version, so lodash@3 gets installed dozens of times for a typical Babel config.

Would it be helpful for me to rebase this PR against master? Are there any other blockers that I can help clear up?

@codecov-io
Copy link

codecov-io commented May 7, 2016

Current coverage is 87.81%

Merging #3315 into master will not change coverage

@@             master      #3315   diff @@
==========================================
  Files           194        194          
  Lines         10119      10119          
  Methods        1535       1535          
  Messages          0          0          
  Branches       2245       2245          
==========================================
  Hits           8886       8886          
  Misses         1233       1233          
  Partials          0          0          

Powered by Codecov. Last updated by 2607f35...7690abd

@hzoo
Copy link
Member

hzoo commented May 13, 2016

Not a big deal for node 4, etc, then. @loganfsmyth

@hzoo hzoo merged commit dc1f405 into babel:master May 13, 2016
@hzoo
Copy link
Member

hzoo commented May 13, 2016

Thanks so much!

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 7, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Internal 🏠 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants