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 v4 #660

Merged
merged 20 commits into from May 14, 2018
Merged

Upgrade to lodash v4 #660

merged 20 commits into from May 14, 2018

Conversation

dominykas
Copy link
Contributor

@dominykas dominykas commented May 8, 2018

Closes #630

How I approached this:

  1. go through each breaking change of lodash v2 to v3 - check every usage of a function that changed (i.e. search by function name)
  2. same for lodash v3 to v4
  3. go through every require of src/lib/utils.js and split up the _ vs utils usage
  4. go through every usage of _. and _( to verify that thisArg is no longer used (breaking change in v4)

Someone handily published a lodash-2 v4.17.4 - it is exactly the same as lodash v4.17.4, so it is safe to use during the migration.
Utils.nextTick with a single argument is the same as process.nextTick
Because it seems that this is the coding style in this repo
@spalger
Copy link
Contributor

spalger commented May 8, 2018

Very cool, @dominykas. Lets keep going with this strategy and merge it once there is only one version of lodash in use, utils have been successfully isolated, and utils no longer extend lodash.

Thanks!

It was a mistake in my previous commit to not update this usage
As all three - gruntUtils, utils and lodash (_) are getting passed into templates, it makes sense to keep the naming consistent
As all three - gruntUtils, utils and lodash (_) are getting passed into templates, it makes sense to keep the naming consistent
Also use lodash-2 where it is easy to do so
package.json Outdated
@@ -106,8 +106,8 @@
"agentkeepalive": "^3.4.1",
"chalk": "^1.0.0",
"lodash": "2.4.2",
"lodash-2": "4.17.4",
Copy link

Choose a reason for hiding this comment

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

Sorry for intruding, but what is this lodash-2 package? With 1 version deployed a year ago and having two weekly downloads on npm, it looks quite fishy to me. I also cannot find any reference to lodash-2 in the official GitHub org. I wouldn't want that package appearing in my code base...

Copy link
Contributor Author

@dominykas dominykas May 10, 2018

Choose a reason for hiding this comment

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

As I mention in the description:

  1. This is pinned to a specific version
  2. If you download the tgz for lodash and lodash-2 for that specific version, you will find they are identical.

That aside, this PR won't go in until I'm done with the migration, and when I'm done - lodash-2 will be gone.

Choose a reason for hiding this comment

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

Oh, I understand, I thought that lodash-2 was to pin a lodash 2.x version and the main lodash package would be upgraded to latest. I guess the the names and versions makes it confusing 😃. Anyways, great work!

@dominykas
Copy link
Contributor Author

OK, this is now done, lodash-2 is cleaned out.

Unit tests are passing.

I'll test this with our internal system, but otherwise I'm not sure how to be certain that I did a good job...

@@ -92,8 +93,7 @@ function Client(config) {
Log: require('./log'),
Logger: require('./logger'),
NodesToHost: require('./nodes_to_host'),
Transport: require('./transport'),
utils: require('./utils')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically, removing this is a breaking change? This doesn't seem to be used internally - but it might be used by plugins?

Even not removing it is probably a breaking change, as utils no longer includes lodash methods and even if it were to include them - lodash v4 methods are breaking, so I'm not sure on the best course of action here?

Copy link
Contributor

Choose a reason for hiding this comment

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

None of the plugins I know about use this, and I agree that it's breaking anyway because it doesn't include lodash anymore. I think you took the best action, remove it and we can always bring it back if necessary.

@dominykas
Copy link
Contributor Author

When I run grunt run:generate, I get a Error: Multiple URLS with the same signature detected for indices.putMapping - but I get the same on 14.x branch, so assuming it's not something I broke?

@dominykas dominykas changed the title Partial lodash upgrade - non-prod code Upgrade to lodash v4 May 11, 2018
@dominykas
Copy link
Contributor Author

Unit tests in our internal system passed (against ES 6.1.3) - I'll check E2E tomorrow or Monday. Of course we don't use that much of the functionality, but still...

@spalger
Copy link
Contributor

spalger commented May 11, 2018

🎉 🎉 Awesome!

I'll take a deeper look Monday and do what I can do double check your efforts. Thank you so much @dominykas!!

@dominykas
Copy link
Contributor Author

Can confirm no issues E2E using this version on our internal system.

@synhershko
Copy link

👍 thanks guys!

Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM, this is super awesome @dominykas, a ton of work, and so very helpful. Thanks again!!

var get = require('lodash.get');
var utils = require('../utils');
var _ = require('lodash');
var gruntUtils = require('../utils');
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for giving gruntUtils and utils clear and distinct identities, and making sure to use them everywhere!

@@ -65,15 +65,15 @@ var config = {
grunt.registerTask('mocha_integration', function (branch) {
grunt.config.set(
'mochacov.integration.src',
'test/integration/yaml_suite/index_' + _.snakeCase(branch) + '.js'
'test/integration/yaml_suite/index_' + utils.snakeCase(branch) + '.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for making that functions that are now a part of lodash, like _.snakeCase, are still using their original implementation to ensure compatibility.

@@ -92,8 +93,7 @@ function Client(config) {
Log: require('./log'),
Logger: require('./logger'),
NodesToHost: require('./nodes_to_host'),
Transport: require('./transport'),
utils: require('./utils')
Copy link
Contributor

Choose a reason for hiding this comment

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

None of the plugins I know about use this, and I agree that it's breaking anyway because it doesn't include lodash anymore. I think you took the best action, remove it and we can always bring it back if necessary.

var utils = rootReq('grunt/utils');
var _ = require('lodash');
var utils = rootReq('src/lib/utils');
var gruntUtils = rootReq('grunt/utils');
Copy link
Contributor

Choose a reason for hiding this comment

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

gruntUtils isn't needed here

@spalger spalger merged commit f1de944 into elastic:14.x May 14, 2018
@dominykas dominykas deleted the lodash-upgrade branch May 15, 2018 07:24
@dominykas
Copy link
Contributor Author

Thanks! Now 🤞I didn't break anything :)

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

4 participants