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

Merged
merged 5 commits into from
Feb 6, 2019
Merged

Conversation

thompsongl
Copy link
Contributor

@thompsongl thompsongl commented Feb 6, 2019

Summary

Resolves #1525.
Gets EUI closer to the aspirations of #360.

Upgrades lodash dependency from pinned Kibana 3.x fork to latest 4.x. Single biggest benefit is cherry-pickable imports (e.g., import uniqBy from 'lodash/uniqBy';).

Where possible (and simple), I removed the lodash dependency entirely from files. Mostly, this involved simple methods that ES can handle natively (e.g., filter, slice, simple array flattening)

EUI's heaviest use of lodash is in services, specifically the predicate utils. As these are part of the public API, they remain unaltered (verified no breaking changes between v3 and v4). Where possible, we now use methods from services rather import directly from lodash.

Checklist

- [ ] This was checked in mobile
- [ ] This was checked in IE11
- [ ] This was checked in dark mode
- [ ] Any props added have proper autodocs
- [ ] Documentation examples were added

  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately

- [ ] Jest tests were updated or added to match the most common scenarios
- [ ] This was checked against keyboard-only and screenreader scenarios
- [ ] This required updates to Framer X components

@thompsongl thompsongl changed the title 1525 lodash upgrade Upgrade lodash to 4.x Feb 6, 2019
@thompsongl thompsongl changed the title Upgrade lodash to 4.x Upgrade lodash to v4 Feb 6, 2019
src/components/form/range/range.js Outdated Show resolved Hide resolved
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

LGTM

@thompsongl thompsongl merged commit a353518 into elastic:master Feb 6, 2019
@nreese
Copy link
Contributor

nreese commented Feb 7, 2019

This is going to cause issues with Kibana. Kibana is on lodash 3.10.1 and an upgrade would be very difficult

@chandlerprall
Copy link
Contributor

@nreese is referring to #359

I ran through a number of pages in Kibana built with EUI+lodash 4, specifically looking at places using EUI tables and EUI range slider (tables were previously the known issue, slider relies on two functions from v4) and hit no issues.

I don't have a definitive answer why it broke previously, but I strongly suspect it was something with npm's dependency resolution (Kibana was not yet on yarn).

@nreese
Copy link
Contributor

nreese commented Feb 7, 2019

Sounds good. Thanks for checking.

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.

Upgrade lodash to v4
3 participants